Display an infobar when content settings were created.

Display an infobar when content settings were created.

The xib changes add a gradient view with an icon and a text field that is displayed as infobar. The text field is set to truncate. The blocked cookies label is set to wrap. The various views are connected to the new outlets.

win: http://imgur.com/OmTxr.png
linux: http://imgur.com/OmTxr
mac: http://imgur.com/VbwFp

BUG=49826
TEST=unit_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56913

[jochen (gone - plz use gerrit), 2010-08-18 16:13:38]

please review.

There's currently a problem. When the info bar is displayed (i.e. set to
visible), the content of the constrained dialog is layouted so everything fits
into the constrained dialog.

What magic do I have to do to grow the constrained dialog before the layout
happens? i.e. instead of the complete dialog growing vertically, the current
content is shrunk so the infobar fits.

[Peter Kasting, 2010-08-18 18:22:44]

http://codereview.chromium.org/3108029/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3108029/diff/1/2#newcode6126
chrome/app/generated_resources.grd:6126: Cookies and site data were set by page
How about "Cookies set by this page"?

The reason I removed "and site data" is because we use "cookies" exclusively in
the dialog content, as well as in most of the rest of Chrome's UI.  The reason I
changed the tense is because the other felt weird as a dialog title.

http://codereview.chromium.org/3108029/diff/1/4
File chrome/app/theme/theme_resources.grd (right):

http://codereview.chromium.org/3108029/diff/1/4#newcode415
chrome/app/theme/theme_resources.grd:415: <include name="IDR_INFO"
file="info_small.png" type="BINDATA" />
Don't add this.  Either use IDR_INPUT_GOOD from app_resources.grd, or
IDR_UPDATE_UPTODATE from this file -- in either case, you can rename as needed.

Another possibility is to add a new icon but have it indicate "information"
(like Windows does with its circular blue "i" badge) instead of good or bad.

I'd check with Glen and/or the other UI leads on the iconography.

http://codereview.chromium.org/3108029/diff/1/5
File chrome/browser/gtk/collected_cookies_gtk.cc (right):

http://codereview.chromium.org/3108029/diff/1/5#newcode16
chrome/browser/gtk/collected_cookies_gtk.cc:16: // Widht and height of the
cookie tree view.
Nit: Width

http://codereview.chromium.org/3108029/diff/1/5#newcode160
chrome/browser/gtk/collected_cookies_gtk.cc:160: if
(host_content_settings_map->BlockThirdPartyCookies()) {
Nit: This could be shorter:

  label = gtk_label_new(l10n_util::GetStringUTF8(
      host_content_settings_map->BlockThirdPartyCookies() ?
      IDS_COLLECTED_COOKIES_BLOCKED_THIRD_PARTY_BLOCKING_ENABLED :
      IDS_COLLECTED_COOKIES_BLOCKED_COOKIES_LABEL).c_str());

http://codereview.chromium.org/3108029/diff/1/7
File chrome/browser/gtk/gtk_tree.h (right):

http://codereview.chromium.org/3108029/diff/1/7#newcode1
chrome/browser/gtk/gtk_tree.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
Nit: 2010

http://codereview.chromium.org/3108029/diff/1/9
File chrome/browser/views/collected_cookies_win.cc (right):

http://codereview.chromium.org/3108029/diff/1/9#newcode56
chrome/browser/views/collected_cookies_win.cc:56: }
Nit: Always a blank line between functions (several places).

http://codereview.chromium.org/3108029/diff/1/9#newcode190
chrome/browser/views/collected_cookies_win.cc:190: if
(host_content_settings_map->BlockThirdPartyCookies()) {
Nit: This can be shortened like in the GTK code.

http://codereview.chromium.org/3108029/diff/1/9#newcode325
chrome/browser/views/collected_cookies_win.cc:325: gfx::Size
CollectedCookiesWin::GetPreferredSize() {
mnissler checked in code (I believe) to allow views to be auto-sized instead of
relying on fixed width and height values.  Assuming that could work here, you
can both get rid of the hardcoded values here and solve your "the dialog doesn't
resize" problems, as that would presumably make GetPreferredSize() return a
different height with and without the infobar visible.

http://codereview.chromium.org/3108029/diff/1/9#newcode331
chrome/browser/views/collected_cookies_win.cc:331: void
CollectedCookiesWin::ChildPreferredSizeChanged(views::View* child) {
Is this actually called?  Perhaps its contents should be in the
AddContentException() function?

http://codereview.chromium.org/3108029/diff/1/10
File chrome/browser/views/collected_cookies_win.h (right):

http://codereview.chromium.org/3108029/diff/1/10#newcode56
chrome/browser/views/collected_cookies_win.h:56: void
ChildPreferredSizeChanged(views::View* child);
This shouldn't go in the views::View implementation block.  Put it under the
constructor or perhaps in a protected: section.

[jochen (gone - plz use gerrit), 2010-08-19 14:45:23]

Addressed all your comments. The Mac version will follow shortly. The win UI now
behaves as I want it to.

[Peter Kasting, 2010-08-19 18:11:27]

http://codereview.chromium.org/3108029/diff/11001/12007
File chrome/browser/views/collected_cookies_win.cc (right):

http://codereview.chromium.org/3108029/diff/11001/12007#newcode56
chrome/browser/views/collected_cookies_win.cc:56: // TODO(jochen): Use an info
icon here instead.
Did you reach the UI leads?  I'm curious what their take is.

http://codereview.chromium.org/3108029/diff/11001/12007#newcode327
chrome/browser/views/collected_cookies_win.cc:327: gfx::Size
size(views::Window::GetLocalizedContentsSize(
Did you look into using mnissler's auto-sizing capabilities so that we can get
rid of these dialog size constants?

http://codereview.chromium.org/3108029/diff/11001/12007#newcode378
chrome/browser/views/collected_cookies_win.cc:378: MapWindowPoints(HWND_DESKTOP,
tab_contents_->GetNativeView(), &topleft, 1);
This won't work on Linux views.  We normally avoid native APIs like this, too. 
Can't you use View::ConvertPointToView() here?

[jochen (gone - plz use gerrit), 2010-08-19 19:31:49]

On 2010/08/19 18:11:27, Peter Kasting wrote:
> http://codereview.chromium.org/3108029/diff/11001/12007
> File chrome/browser/views/collected_cookies_win.cc (right):
> 
> http://codereview.chromium.org/3108029/diff/11001/12007#newcode56
> chrome/browser/views/collected_cookies_win.cc:56: // TODO(jochen): Use an info
> icon here instead.
> Did you reach the UI leads?  I'm curious what their take is.

I met with Brian an hour ago and demoed the UI to him. I also mentioned that the
icon should be replaced with a better one. While he was positive about the UI,
he deferred the icon thing to Glen. I'll try to get him to comment later.

> 
> http://codereview.chromium.org/3108029/diff/11001/12007#newcode327
> chrome/browser/views/collected_cookies_win.cc:327: gfx::Size
> size(views::Window::GetLocalizedContentsSize(
> Did you look into using mnissler's auto-sizing capabilities so that we can get
> rid of these dialog size constants?

We'll always need to have constants for the size of the tree views.

> 
> http://codereview.chromium.org/3108029/diff/11001/12007#newcode378
> chrome/browser/views/collected_cookies_win.cc:378:
MapWindowPoints(HWND_DESKTOP,
> tab_contents_->GetNativeView(), &topleft, 1);
> This won't work on Linux views.  We normally avoid native APIs like this, too.

> Can't you use View::ConvertPointToView() here?

I'll try this. This dialog is windows only. Linux Views displays the gtk
version.

[Peter Kasting, 2010-08-19 20:28:04]

On 2010/08/19 19:31:49, jochen wrote:
> > http://codereview.chromium.org/3108029/diff/11001/12007#newcode327
> > chrome/browser/views/collected_cookies_win.cc:327: gfx::Size
> > size(views::Window::GetLocalizedContentsSize(
> > Did you look into using mnissler's auto-sizing capabilities so that we can
get
> > rid of these dialog size constants?
> 
> We'll always need to have constants for the size of the tree views.

Sure, but that seems better than sizing the whole dialog...

I guess it's not a huge deal, just seems like it might end up nicer in the end.

[jochen (gone - plz use gerrit), 2010-08-20 12:09:50]

please review.

I agreed on changing reviewers with Peter since he didn't have any further major
concerns, and I wanted to get stuff done during daytime.

Mattias, please have a look at the windows and gtk stuff.

Bernhard, I'll shortly upload a new version of the Mac stuff.

I'll add screenshots of the UI on all platforms to the description soon.

[Bernhard Bauer, 2010-08-20 12:19:19]

On 2010/08/20 12:09:50, jochen wrote:
> please review.
> 
> I agreed on changing reviewers with Peter since he didn't have any further
major
> concerns, and I wanted to get stuff done during daytime.
> 
> Mattias, please have a look at the windows and gtk stuff.
> 
> Bernhard, I'll shortly upload a new version of the Mac stuff.

Does that mean I should wait with reviewing?

[Bernhard Bauer, 2010-08-20 13:17:12]

Mac stuff LGTM so far with two nits.

Also, please add a description of the XIB changes.

http://codereview.chromium.org/3108029/diff/20001/21004
File chrome/browser/cocoa/collected_cookies_mac.mm (right):

http://codereview.chromium.org/3108029/diff/20001/21004#newcode26
chrome/browser/cocoa/collected_cookies_mac.mm:26: static const double
kBannerGradientColorTop[3] =
Nit: remove unnecessary static modifier here.

http://codereview.chromium.org/3108029/diff/20001/21004#newcode117
chrome/browser/cocoa/collected_cookies_mac.mm:117:
andSetting:(ContentSetting)setting;
Nit: Usually the signature would be just showInfoBarForDomain:setting: without
the "and".

[Mattias Nissler (ping if slow), 2010-08-20 13:25:08]

A couple of comments/nits to resolve.

http://codereview.chromium.org/3108029/diff/20001/21005
File chrome/browser/gtk/collected_cookies_gtk.cc (right):

http://codereview.chromium.org/3108029/diff/20001/21005#newcode17
chrome/browser/gtk/collected_cookies_gtk.cc:17: const int kTreeViewWidth = 450;
Maybe kDialogContentWidth would be a more appropriate name?

http://codereview.chromium.org/3108029/diff/20001/21005#newcode237
chrome/browser/gtk/collected_cookies_gtk.cc:237: infobar_label_ =
gtk_label_new("test");
test? I like NULL better.

http://codereview.chromium.org/3108029/diff/20001/21005#newcode354
chrome/browser/gtk/collected_cookies_gtk.cc:354: g_list_foreach(paths,
reinterpret_cast<GFunc>(gtk_tree_path_free), NULL);
is it ok the delete here even if !node->CanCreateContentException()?

http://codereview.chromium.org/3108029/diff/20001/21005#newcode361
chrome/browser/gtk/collected_cookies_gtk.cc:361: setting,
multiple_domains_added, last_domain_name).c_str());
I think you need another 4 spaces of indenting here.

http://codereview.chromium.org/3108029/diff/20001/21009
File chrome/browser/views/collected_cookies_win.cc (right):

http://codereview.chromium.org/3108029/diff/20001/21009#newcode124
chrome/browser/views/collected_cookies_win.cc:124: 0,
kRelatedControlVerticalSpacing - 1,
why -1 ? If kRelatedControlVerticalSpacing is 0, you get -1 here, which isn't
right.

http://codereview.chromium.org/3108029/diff/20001/21009#newcode330
chrome/browser/views/collected_cookies_win.cc:330:
IDS_COOKIES_DIALOG_HEIGHT_LINES));
Hm, you use a layout here, how about just asking the layout for a suitable size?
This is kind of crude.

http://codereview.chromium.org/3108029/diff/20001/21009#newcode334
chrome/browser/views/collected_cookies_win.cc:334: size.Enlarge(0,
infobar_size.height());
To get this really correct, you'd also need a 

size.set_width(max(size.width(), infobar_size.width))

But as I commented above, autosizing should work just fine (if you have the
preferred sizes for your child components set correctly).

http://codereview.chromium.org/3108029/diff/20001/21009#newcode374
chrome/browser/views/collected_cookies_win.cc:374: InfobarView* real_infobar =
static_cast<InfobarView*>(infobar_);
This is ugly. How about a forward declaration in CollectedCookiesWin?

http://codereview.chromium.org/3108029/diff/20001/21009#newcode379
chrome/browser/views/collected_cookies_win.cc:379: MapWindowPoints(HWND_DESKTOP,
tab_contents_->GetNativeView(), &topleft, 1);
There's a ClientToScreen call in WidgetWin::GetBounds(). So you're translating
twice? Why is that necessary? Why does it actually work?

[jochen (gone - plz use gerrit), 2010-08-20 14:04:12]

http://codereview.chromium.org/3108029/diff/20001/21005
File chrome/browser/gtk/collected_cookies_gtk.cc (right):

http://codereview.chromium.org/3108029/diff/20001/21005#newcode17
chrome/browser/gtk/collected_cookies_gtk.cc:17: const int kTreeViewWidth = 450;
On 2010/08/20 13:25:08, mnissler wrote:
> Maybe kDialogContentWidth would be a more appropriate name?
the dialog is wider, this is really only applied to the treeview itself

http://codereview.chromium.org/3108029/diff/20001/21005#newcode237
chrome/browser/gtk/collected_cookies_gtk.cc:237: infobar_label_ =
gtk_label_new("test");
On 2010/08/20 13:25:08, mnissler wrote:
> test? I like NULL better.

Done.

http://codereview.chromium.org/3108029/diff/20001/21005#newcode354
chrome/browser/gtk/collected_cookies_gtk.cc:354: g_list_foreach(paths,
reinterpret_cast<GFunc>(gtk_tree_path_free), NULL);
On 2010/08/20 13:25:08, mnissler wrote:
> is it ok the delete here even if !node->CanCreateContentException()?

this is required to free the tree selection (which is allocated before the loop)

http://codereview.chromium.org/3108029/diff/20001/21005#newcode361
chrome/browser/gtk/collected_cookies_gtk.cc:361: setting,
multiple_domains_added, last_domain_name).c_str());
On 2010/08/20 13:25:08, mnissler wrote:
> I think you need another 4 spaces of indenting here.

Done.

http://codereview.chromium.org/3108029/diff/20001/21009
File chrome/browser/views/collected_cookies_win.cc (right):

http://codereview.chromium.org/3108029/diff/20001/21009#newcode330
chrome/browser/views/collected_cookies_win.cc:330:
IDS_COOKIES_DIALOG_HEIGHT_LINES));
On 2010/08/20 13:25:08, mnissler wrote:
> Hm, you use a layout here, how about just asking the layout for a suitable
size?
> This is kind of crude.

Done.

http://codereview.chromium.org/3108029/diff/20001/21009#newcode374
chrome/browser/views/collected_cookies_win.cc:374: InfobarView* real_infobar =
static_cast<InfobarView*>(infobar_);
On 2010/08/20 13:25:08, mnissler wrote:
> This is ugly. How about a forward declaration in CollectedCookiesWin?

Done.

http://codereview.chromium.org/3108029/diff/20001/21009#newcode379
chrome/browser/views/collected_cookies_win.cc:379: MapWindowPoints(HWND_DESKTOP,
tab_contents_->GetNativeView(), &topleft, 1);
On 2010/08/20 13:25:08, mnissler wrote:
> There's a ClientToScreen call in WidgetWin::GetBounds(). So you're translating
> twice? Why is that necessary? Why does it actually work?

This call seems to make the coordinates relative to the hwnd of the tab
contents. The SetBounds call expects coordinates relative to the desktop.

[Mattias Nissler (ping if slow), 2010-08-20 14:51:30]

LGTM with nits.

http://codereview.chromium.org/3108029/diff/20001/21005
File chrome/browser/gtk/collected_cookies_gtk.cc (right):

http://codereview.chromium.org/3108029/diff/20001/21005#newcode17
chrome/browser/gtk/collected_cookies_gtk.cc:17: const int kTreeViewWidth = 450;
On 2010/08/20 14:04:12, jochen wrote:
> On 2010/08/20 13:25:08, mnissler wrote:
> > Maybe kDialogContentWidth would be a more appropriate name?
> the dialog is wider, this is really only applied to the treeview itself
> 

OK, and that label kind of belongs to the tree ;)

http://codereview.chromium.org/3108029/diff/20001/21005#newcode354
chrome/browser/gtk/collected_cookies_gtk.cc:354: g_list_foreach(paths,
reinterpret_cast<GFunc>(gtk_tree_path_free), NULL);
On 2010/08/20 14:04:12, jochen wrote:
> On 2010/08/20 13:25:08, mnissler wrote:
> > is it ok the delete here even if !node->CanCreateContentException()?
> 
> this is required to free the tree selection (which is allocated before the
loop)

Ah, right, I see :)

http://codereview.chromium.org/3108029/diff/20001/21009
File chrome/browser/views/collected_cookies_win.cc (right):

http://codereview.chromium.org/3108029/diff/20001/21009#newcode379
chrome/browser/views/collected_cookies_win.cc:379: MapWindowPoints(HWND_DESKTOP,
tab_contents_->GetNativeView(), &topleft, 1);
On 2010/08/20 14:04:12, jochen wrote:
> On 2010/08/20 13:25:08, mnissler wrote:
> > There's a ClientToScreen call in WidgetWin::GetBounds(). So you're
translating
> > twice? Why is that necessary? Why does it actually work?
> 
> This call seems to make the coordinates relative to the hwnd of the tab
> contents. The SetBounds call expects coordinates relative to the desktop.
> 

I'd consider that a bug in views. SetBounds and GetBounds should always work in
the same coordinate system. Please put a comment here that explains the problem
and file a bug for it.

http://codereview.chromium.org/3108029/diff/13002/27010#newcode196
chrome/browser/views/collected_cookies_win.cc:196: l10n_util::GetString(
Calling it twice just to be sure? Maybe you want to add a loop that only breaks
if the value read back is false ;)

[Bernhard Bauer, 2010-08-20 15:29:07]

LGTM with one last nit fixed:

http://codereview.chromium.org/3108029/diff/13002/27005
File chrome/browser/cocoa/collected_cookies_mac.mm (right):

http://codereview.chromium.org/3108029/diff/13002/27005#newcode118
chrome/browser/cocoa/collected_cookies_mac.mm:118:
Setting:(ContentSetting)setting;
'nother nit: write setting: small here.

[jochen (gone - plz use gerrit), 2010-08-20 15:34:18]

ok, all nits addressed.