UI polish for certificate viewer

UI polish for certificate viewer(except chromeos)

Deprecated. The base URL is wrong. Created an identical CL to fix the base URL http://codereview.chromium.org/8461015/

This CL makes certificate viewer for chromeos looks like the screen shot posted in issue 102511. It hacks bubble_frame_view.cc to get rid of the body padding and add a x button in the title area.

It should not affect the appearance of any other html dialogs.

BUG=102511
TEST=Go to https://www.google.com; Click the locker icon on the left side of URL in the omnibox and click "certificate information". For screenshots, see http://code.google.com/p/chromium/issues/detail?id=102511

[bshe, 2011-11-07 20:17:55]

Hey Rick.

Could you take a look at this short CL? It includes the UI polish for
certificate viewers. There are still problems on chrome os. I will file a new
bug and dig more into it.

Thanks,

Biao

[Rick Byers, 2011-11-07 21:43:47]

Looks good.

Please verify that's there's no other mainline use of tabs.css that you may be
changing (internal/diagnostics pages shouldn't require UI review), and spot
check a couple with your changes to make sure they still look ok after your
changes.

http://codereview.chromium.org/8479042/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/tabs.css (right):

http://codereview.chromium.org/8479042/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/tabs.css:34: background: #eee;
This should go above in the "tabs > *" clause (between this rule and the one
below, you're overriding the background in all cases).

It looks like it's using a partially transparent background today (to let a
little bit of the gradient show through) - glen probably assumed it was opaque. 
I don't think it makes a big difference though - try rgba(238, 238, 238, .3) and
make sure that looks like his mocks.

You should also update the border color to match.

Also the hover state will probably now look weird (hovering previously lightened
it, now it will darken it).  Try something in the middle - eg.
rgba(247,247,247,.3) and check to make sure you think it looks OK.  We can
confirm with Glen later.

[bshe, 2011-11-09 22:36:54]

I edited this issue description since it contains all the changes for issue
102511 now.

This patch addressed the comments from Rick with some further css tweak due to
the removal of close button. It also included the hack for certificate viewer on
chromeos. I have compiled and tested it on linux and chromeos with/without
touchui enabled. It looks good. Didn't tested it on Mac and Windows though.

It's a pretty ugly hack and I am sure there are better ways to do it. Just want
to check in first patch which at least works.

Thanks,

http://codereview.chromium.org/8479042/diff/1/chrome/browser/resources/shared...
File chrome/browser/resources/shared/css/tabs.css (right):

http://codereview.chromium.org/8479042/diff/1/chrome/browser/resources/shared...
chrome/browser/resources/shared/css/tabs.css:34: background: #eee;
On 2011/11/07 21:43:48, Rick Byers wrote:
> This should go above in the "tabs > *" clause (between this rule and the one
> below, you're overriding the background in all cases).
> 
> It looks like it's using a partially transparent background today (to let a
> little bit of the gradient show through) - glen probably assumed it was
opaque. 
> I don't think it makes a big difference though - try rgba(238, 238, 238, .3)
and
> make sure that looks like his mocks.
> 
> You should also update the border color to match.
> 
> Also the hover state will probably now look weird (hovering previously
lightened
> it, now it will darken it).  Try something in the middle - eg.
> rgba(247,247,247,.3) and check to make sure you think it looks OK.  We can
> confirm with Glen later.

Done.

[Rick Byers, 2011-11-10 16:01:00]

On 2011/11/09 22:36:54, bshe wrote:
> I edited this issue description since it contains all the changes for issue
> 102511 now.
> 
> This patch addressed the comments from Rick with some further css tweak due to
> the removal of close button. It also included the hack for certificate viewer
on
> chromeos. I have compiled and tested it on linux and chromeos with/without
> touchui enabled. It looks good. Didn't tested it on Mac and Windows though.

The updates for my comments look good.  I'll leave it to Rob to review all the
new stuff.

[flackr, 2011-11-10 16:03:16]

We should definitely not have a different function signature on specific
platforms. I think the right way to introduce this change is to add a generic
WindowStyle which may only have STYLE_GENERIC for other OS's (for now), but will
have extra styles available for OS_CHROMEOS. Then all calls to ShowHtmlDialog
will have to be changed to pass this generic style. BubbleWindow will have to be
changed to use the generic style instead of having it's own specific one which
may mean lots of changes to BubbleWindow related code.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/bubble_frame_view.cc (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_frame_view.cc:32: const int
kTitleRightPadding = 10;
I don't think there's a need for these extra contents, most likely we will
always want the title to be padded the same as if we didn't apply this new style
(i.e. re-use kHorizontalPadding.)

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_frame_view.cc:63: if (style_ & STYLE_XBAR
|| style_ & STYLE_CERT) {
Your new style should only be for the custom padding.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_frame_view.cc:184:
throbber_size.height()));
The following cases for STYLE_CERT seem to introduce a lot of duplicate code. I
suggest you set up gfx::Insets titleBarInsets here and then you can use it in
the following code and not require two cases.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_frame_view.cc:191: insets.top() +
((max_height - title_size.height()) >> 1), // Center
Are divisions by 2 of this style (>> 1) used elsewhere? I think any compiler
will have no trouble optimizing an integer divide by constant 2 to this and it
will be more readable to just divide by 2.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/bubble_window_style.h (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_window_style.h:31: STYLE_CERT = 1 << 3
This style probably isn't going to be specific to certificate viewer, and even
if it is it should have a generic name describing what it does. It's very
similar to STYLE_FLUSH except that you still add the padding to the title bar. I
think a name like STYLE_FLUSH_CONTENT would be better suited.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/resources/cer...
File chrome/browser/resources/certificate_viewer.css (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/resources/cer...
chrome/browser/resources/certificate_viewer.css:60: }
Remove this, no longer used.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:459: gfx::NativeWindow
BrowserShowHtmlDialog(HtmlDialogUIDelegate* delegate,
Unfortunately I am fairly certain that having a different function signature for
OS_CHROMEOS is not going to be accepted.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:462: chromeos::STYLE_GENERIC);
Default argument values are not allowed according to the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/webui/cert...
File chrome/browser/ui/webui/certificate_viewer.cc (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/webui/cert...
chrome/browser/ui/webui/certificate_viewer.cc:86: window_ =
browser->BrowserShowHtmlDialog(this, parent, style_);
Assuming the rename I suggested earlier and using the existing style to toggle
the close button, this would be STYLE_XBAR | STYLE_FLUSH_CONTENT

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/webui/cert...
File chrome/browser/ui/webui/certificate_viewer.h (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/webui/cert...
chrome/browser/ui/webui/certificate_viewer.h:73: #endif
Don't add style as a parameter to the class, just add it to the window creation
call.

[bshe, 2011-11-21 16:09:37]

Hi Rob.

Could you please take a look at this CL? It addresses all the comments you made
and also includes some css changes for two internal pages.

I have created an identical CL to fix the base URL problem. The URL is
http://codereview.chromium.org/8461015
Could you please make comments in the new CL? Sorry for inconvenience. Thanks!

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/bubble_frame_view.cc (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_frame_view.cc:32: const int
kTitleRightPadding = 10;
On 2011/11/10 16:03:16, flackr wrote:
> I don't think there's a need for these extra contents, most likely we will
> always want the title to be padded the same as if we didn't apply this new
style
> (i.e. re-use kHorizontalPadding.)

Done.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_frame_view.cc:63: if (style_ & STYLE_XBAR
|| style_ & STYLE_CERT) {
On 2011/11/10 16:03:16, flackr wrote:
> Your new style should only be for the custom padding.

Done.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_frame_view.cc:184:
throbber_size.height()));
On 2011/11/10 16:03:16, flackr wrote:
> The following cases for STYLE_CERT seem to introduce a lot of duplicate code.
I
> suggest you set up gfx::Insets titleBarInsets here and then you can use it in
> the following code and not require two cases.

I use titleBarInsets at first. Then I figure out the XBAR style already adds
padding to title. We just need to offset the content area padding in this case.
So instead of using title padding, I use an gfx::Insets offset_insets to flush
content area.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_frame_view.cc:191: insets.top() +
((max_height - title_size.height()) >> 1), // Center
On 2011/11/10 16:03:16, flackr wrote:
> Are divisions by 2 of this style (>> 1) used elsewhere? I think any compiler
> will have no trouble optimizing an integer divide by constant 2 to this and it
> will be more readable to just divide by 2.

Done.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
File chrome/browser/chromeos/frame/bubble_window_style.h (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/chromeos/fram...
chrome/browser/chromeos/frame/bubble_window_style.h:31: STYLE_CERT = 1 << 3
On 2011/11/10 16:03:16, flackr wrote:
> This style probably isn't going to be specific to certificate viewer, and even
> if it is it should have a generic name describing what it does. It's very
> similar to STYLE_FLUSH except that you still add the padding to the title bar.
I
> think a name like STYLE_FLUSH_CONTENT would be better suited.

Move the style to chrome/brwoser/ui/dialog_style.h. Add a
"STYLE_XBAR_FLUSH_CONTENT". Since we also have a Xbar, it makes more sense to
add XBAR to the style name.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/resources/cer...
File chrome/browser/resources/certificate_viewer.css (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/resources/cer...
chrome/browser/resources/certificate_viewer.css:60: }
On 2011/11/10 16:03:16, flackr wrote:
> Remove this, no longer used.

Done.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:459: gfx::NativeWindow
BrowserShowHtmlDialog(HtmlDialogUIDelegate* delegate,
On 2011/11/10 16:03:16, flackr wrote:
> Unfortunately I am fairly certain that having a different function signature
for
> OS_CHROMEOS is not going to be accepted.

Done.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:462: chromeos::STYLE_GENERIC);
On 2011/11/10 16:03:16, flackr wrote:
> Default argument values are not allowed according to the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

Done. Thanks for the link!

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/webui/cert...
File chrome/browser/ui/webui/certificate_viewer.cc (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/webui/cert...
chrome/browser/ui/webui/certificate_viewer.cc:86: window_ =
browser->BrowserShowHtmlDialog(this, parent, style_);
On 2011/11/10 16:03:16, flackr wrote:
> Assuming the rename I suggested earlier and using the existing style to toggle
> the close button, this would be STYLE_XBAR | STYLE_FLUSH_CONTENT

Done.

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/webui/cert...
File chrome/browser/ui/webui/certificate_viewer.h (right):

http://codereview.chromium.org/8479042/diff/5001/chrome/browser/ui/webui/cert...
chrome/browser/ui/webui/certificate_viewer.h:73: #endif
On 2011/11/10 16:03:16, flackr wrote:
> Don't add style as a parameter to the class, just add it to the window
creation
> call.

Done.

[flackr, 2011-11-21 20:43:30]

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
File chrome/browser/chromeos/frame/bubble_frame_view.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.cc:151: kHorizontalPadding);
I think the Insets you return here should not include the horizontal padding.
According to views/view.h this returns the insets of the current border. In the
case of FLUSH_CONTENT the overall border does not include the horizontal
padding.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.cc:191: insets.top() +
(static_cast<int>(
Remove static_cast, integer division results in an integer.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.cc:200: insets.top() +
(static_cast<int>(
ditto

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.cc:208: insets.top() +
(static_cast<int> (
ditto

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.cc:223: gfx::Insets
offset_insets = GetOffsetInsets();
As per my above comment, my preference would be if you return the insets without
horizontal padding from GetInsets, and then you can use a local variable to
customize the insets for the titlebar, close button, and throbber above.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
File chrome/browser/chromeos/frame/bubble_frame_view.h (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.h:60: gfx::Insets
GetOffsetInsets() const;
Don't need extra method for single use.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:493: STYLE_GENERIC);
One line per argument once you have to wrap.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/ce...
File chrome/browser/resources/certificate_viewer.css (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.css:8: margin: 8px 0 0 0;
nit: sort.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.css:98: margin-bottom: 10px;
nit: sort

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/qu...
File chrome/browser/resources/quota_internals/main.html (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/qu...
chrome/browser/resources/quota_internals/main.html:14: }
Move to CSS file.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/css/tabs.css (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sh...
chrome/browser/resources/shared/css/tabs.css:11: -webkit-padding-start: 8px;
nit: Move to top, according to sorting order and established convention.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/sync_index.html (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_index.html:16: }
Move to CSS file.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/dialog_st...
File chrome/browser/ui/dialog_style.h (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/dialog_st...
chrome/browser/ui/dialog_style.h:30: // Only for certificate viewer. Add close
button to the top right and fluch
The style presumably only makes the content flush to edge, but does not add a
close button. To differentiate it from STYLE_FLUSH above mention that the title
bar will still have the regular padding. Also, remove the Only for certificate
viewer comment. We were discussing with rbyers whether we could change FLUSH to
mean what we wanted. Have you checked if any dialogs that use STYLE_FLUSH also
have titles? If not we don't even have to introduce a new style for this.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/gtk/html_...
File chrome/browser/ui/gtk/html_dialog_gtk.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/gtk/html_...
chrome/browser/ui/gtk/html_dialog_gtk.cc:30: // in html_dialog_view.cc file.
TODO: Add style parameter to HtmlDialogGtk.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/views/abo...
File chrome/browser/ui/views/about_chrome_view.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/views/abo...
chrome/browser/ui/views/about_chrome_view.cc:95: STYLE_GENERIC);
One argument per line.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:122:
STYLE_GENERIC)->Show();
One argument per line.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/cer...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/cer...
chrome/browser/ui/webui/certificate_viewer_webui.cc:86: window_ =
ConstrainedHtmlUI::CreateConstrainedHtmlDialog(
Have you checked how this looks on AURA yet?

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:190:
take_photo_dialog, STYLE_GENERIC);
One argument per line.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/clo...
File chrome/browser/ui/webui/cloud_print_signin_dialog.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/clo...
chrome/browser/ui/webui/cloud_print_signin_dialog.cc:185: STYLE_GENERIC);
One argument per line.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc
(right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc:165:
new TakePhotoDialog(this), STYLE_GENERIC);
One argument per line.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:902:
STYLE_GENERIC);
One argument per line.

[bshe, 2011-11-22 16:26:24]

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
File chrome/browser/chromeos/frame/bubble_frame_view.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.cc:151: kHorizontalPadding);
On 2011/11/21 20:43:31, flackr wrote:
> I think the Insets you return here should not include the horizontal padding.
> According to views/view.h this returns the insets of the current border. In
the
> case of FLUSH_CONTENT the overall border does not include the horizontal
> padding.

I was thinking that I can use offset_insets to offset the horizontal padding of
content. So that I just need to change the code in one place(content) instead of
three places(title, x_button, throbber). But you are right. I should conform
with the description of this function in views/view.h. Less hacky.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.cc:191: insets.top() +
(static_cast<int>(
On 2011/11/21 20:43:31, flackr wrote:
> Remove static_cast, integer division results in an integer.
Done. 
A stupid mistake...

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.cc:200: insets.top() +
(static_cast<int>(
On 2011/11/21 20:43:31, flackr wrote:
> ditto

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.cc:208: insets.top() +
(static_cast<int> (
On 2011/11/21 20:43:31, flackr wrote:
> ditto

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.cc:223: gfx::Insets
offset_insets = GetOffsetInsets();
On 2011/11/21 20:43:31, flackr wrote:
> As per my above comment, my preference would be if you return the insets
without
> horizontal padding from GetInsets, and then you can use a local variable to
> customize the insets for the titlebar, close button, and throbber above.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
File chrome/browser/chromeos/frame/bubble_frame_view.h (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/fra...
chrome/browser/chromeos/frame/bubble_frame_view.h:60: gfx::Insets
GetOffsetInsets() const;
On 2011/11/21 20:43:31, flackr wrote:
> Don't need extra method for single use.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:493: STYLE_GENERIC);
On 2011/11/21 20:43:31, flackr wrote:
> One line per argument once you have to wrap.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/ce...
File chrome/browser/resources/certificate_viewer.css (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.css:8: margin: 8px 0 0 0;
On 2011/11/21 20:43:31, flackr wrote:
> nit: sort.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/ce...
chrome/browser/resources/certificate_viewer.css:98: margin-bottom: 10px;
On 2011/11/21 20:43:31, flackr wrote:
> nit: sort

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/qu...
File chrome/browser/resources/quota_internals/main.html (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/qu...
chrome/browser/resources/quota_internals/main.html:14: }
On 2011/11/21 20:43:31, flackr wrote:
> Move to CSS file.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/css/tabs.css (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sh...
chrome/browser/resources/shared/css/tabs.css:11: -webkit-padding-start: 8px;
On 2011/11/21 20:43:31, flackr wrote:
> nit: Move to top, according to sorting order and established convention.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/sync_index.html (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_index.html:16: }
On 2011/11/21 20:43:31, flackr wrote:
> Move to CSS file.
Should I create a new CSS file for this rule? Or should I put this rule to
sync_search.css/sync_node_browser.css file? sync_search.css and
sync_node_browser.css dont seem like the right place to put this rule. They are
for search and node_browser tabs only. Also not sure create a new css file with
just one rule is good or bad?

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/dialog_st...
File chrome/browser/ui/dialog_style.h (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/dialog_st...
chrome/browser/ui/dialog_style.h:30: // Only for certificate viewer. Add close
button to the top right and fluch
On 2011/11/21 20:43:31, flackr wrote:
> The style presumably only makes the content flush to edge, but does not add a
> close button. To differentiate it from STYLE_FLUSH above mention that the
title
> bar will still have the regular padding. Also, remove the Only for certificate
> viewer comment. We were discussing with rbyers whether we could change FLUSH
to
> mean what we wanted. Have you checked if any dialogs that use STYLE_FLUSH also
> have titles? If not we don't even have to introduce a new style for this.

Done. 

We will keep this style as extension dialog uses STYLE_FLUSH and has title.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/gtk/html_...
File chrome/browser/ui/gtk/html_dialog_gtk.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/gtk/html_...
chrome/browser/ui/gtk/html_dialog_gtk.cc:30: // in html_dialog_view.cc file.
On 2011/11/21 20:43:31, flackr wrote:
> TODO: Add style parameter to HtmlDialogGtk.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/views/abo...
File chrome/browser/ui/views/about_chrome_view.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/views/abo...
chrome/browser/ui/views/about_chrome_view.cc:95: STYLE_GENERIC);
On 2011/11/21 20:43:31, flackr wrote:
> One argument per line.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc
(right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_uninstall_dialog_view.cc:122:
STYLE_GENERIC)->Show();
On 2011/11/21 20:43:31, flackr wrote:
> One argument per line.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/cer...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/cer...
chrome/browser/ui/webui/certificate_viewer_webui.cc:86: window_ =
ConstrainedHtmlUI::CreateConstrainedHtmlDialog(
On 2011/11/21 20:43:31, flackr wrote:
> Have you checked how this looks on AURA yet?

Yes I have. Overall, it doesn't look bad. The aura window has a default grey
outer border and the title doesn't align with tabbox. We might need some UI
tweaks for Aura.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc
(right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/user_image_screen_handler.cc:190:
take_photo_dialog, STYLE_GENERIC);
On 2011/11/21 20:43:31, flackr wrote:
> One argument per line.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/clo...
File chrome/browser/ui/webui/cloud_print_signin_dialog.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/clo...
chrome/browser/ui/webui/cloud_print_signin_dialog.cc:185: STYLE_GENERIC);
On 2011/11/21 20:43:31, flackr wrote:
> One argument per line.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc
(right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc:165:
new TakePhotoDialog(this), STYLE_GENERIC);
On 2011/11/21 20:43:31, flackr wrote:
> One argument per line.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:902:
STYLE_GENERIC);
On 2011/11/21 20:43:31, flackr wrote:
> One argument per line.

Done.

[flackr, 2011-11-22 17:02:59]

This is looking great, very close!

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/sync_index.html (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_index.html:16: }
On 2011/11/22 16:26:24, bshe wrote:
> On 2011/11/21 20:43:31, flackr wrote:
> > Move to CSS file.
> Should I create a new CSS file for this rule? Or should I put this rule to
> sync_search.css/sync_node_browser.css file? sync_search.css and
> sync_node_browser.css dont seem like the right place to put this rule. They
are
> for search and node_browser tabs only. Also not sure create a new css file
with
> just one rule is good or bad?

It's okay to create a new file with one rule, if there are more overall rules
for the page then it can be further used. As you say, both the existing css
files are specific to one tab, so adding a general file for the overall page
makes sense :-)

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/dialog_st...
File chrome/browser/ui/dialog_style.h (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/dialog_st...
chrome/browser/ui/dialog_style.h:30: // Only for certificate viewer. Add close
button to the top right and fluch
On 2011/11/22 16:26:24, bshe wrote:
> On 2011/11/21 20:43:31, flackr wrote:
> > The style presumably only makes the content flush to edge, but does not add
a
> > close button. To differentiate it from STYLE_FLUSH above mention that the
> title
> > bar will still have the regular padding. Also, remove the Only for
certificate
> > viewer comment. We were discussing with rbyers whether we could change FLUSH
> to
> > mean what we wanted. Have you checked if any dialogs that use STYLE_FLUSH
also
> > have titles? If not we don't even have to introduce a new style for this.
> 
> Done. 
> 
> We will keep this style as extension dialog uses STYLE_FLUSH and has title.

Looks good, change this comment to padding only on title to be extra clear about
the difference.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/cer...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/cer...
chrome/browser/ui/webui/certificate_viewer_webui.cc:86: window_ =
ConstrainedHtmlUI::CreateConstrainedHtmlDialog(
On 2011/11/22 16:26:24, bshe wrote:
> On 2011/11/21 20:43:31, flackr wrote:
> > Have you checked how this looks on AURA yet?
> 
> Yes I have. Overall, it doesn't look bad. The aura window has a default grey
> outer border and the title doesn't align with tabbox. We might need some UI
> tweaks for Aura. 

Great, thanks! That can of course be done separately, couldn't hurt to leave a
TODO behind for it though. :-)

[bshe, 2011-11-22 19:20:45]

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sy...
File chrome/browser/resources/sync_internals/sync_index.html (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/resources/sy...
chrome/browser/resources/sync_internals/sync_index.html:16: }
On 2011/11/22 17:02:59, flackr wrote:
> On 2011/11/22 16:26:24, bshe wrote:
> > On 2011/11/21 20:43:31, flackr wrote:
> > > Move to CSS file.
> > Should I create a new CSS file for this rule? Or should I put this rule to
> > sync_search.css/sync_node_browser.css file? sync_search.css and
> > sync_node_browser.css dont seem like the right place to put this rule. They
> are
> > for search and node_browser tabs only. Also not sure create a new css file
> with
> > just one rule is good or bad?
> 
> It's okay to create a new file with one rule, if there are more overall rules
> for the page then it can be further used. As you say, both the existing css
> files are specific to one tab, so adding a general file for the overall page
> makes sense :-)

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/dialog_st...
File chrome/browser/ui/dialog_style.h (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/dialog_st...
chrome/browser/ui/dialog_style.h:30: // Only for certificate viewer. Add close
button to the top right and fluch
On 2011/11/22 17:02:59, flackr wrote:
> On 2011/11/22 16:26:24, bshe wrote:
> > On 2011/11/21 20:43:31, flackr wrote:
> > > The style presumably only makes the content flush to edge, but does not
add
> a
> > > close button. To differentiate it from STYLE_FLUSH above mention that the
> > title
> > > bar will still have the regular padding. Also, remove the Only for
> certificate
> > > viewer comment. We were discussing with rbyers whether we could change
FLUSH
> > to
> > > mean what we wanted. Have you checked if any dialogs that use STYLE_FLUSH
> also
> > > have titles? If not we don't even have to introduce a new style for this.
> > 
> > Done. 
> > 
> > We will keep this style as extension dialog uses STYLE_FLUSH and has title.
> 
> Looks good, change this comment to padding only on title to be extra clear
about
> the difference.

Done.

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/cer...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

http://codereview.chromium.org/8479042/diff/25001/chrome/browser/ui/webui/cer...
chrome/browser/ui/webui/certificate_viewer_webui.cc:86: window_ =
ConstrainedHtmlUI::CreateConstrainedHtmlDialog(
On 2011/11/22 17:02:59, flackr wrote:
> On 2011/11/22 16:26:24, bshe wrote:
> > On 2011/11/21 20:43:31, flackr wrote:
> > > Have you checked how this looks on AURA yet?
> > 
> > Yes I have. Overall, it doesn't look bad. The aura window has a default grey
> > outer border and the title doesn't align with tabbox. We might need some UI
> > tweaks for Aura. 
> 
> Great, thanks! That can of course be done separately, couldn't hurt to leave a
> TODO behind for it though. :-)

Done.