Preliminary implementation of Google-style dialogs in ash::internal::DialogFrameView

Preliminary implementation of Google-style dialogs in ash::internal::DialogFrameView

This code is only compiled into Aura and only used if the "Google-style
dialogs" flag is specified (--aura-google-dialog-frames).

Remaining issues:

* Constrained windows are still broken -- see issue 109138.

* Title and body text should be #222, not black. This CL only affects the
  title, and currently leaves it at black to match the body text.

* The shadow and border stroke are not up to spec. Fixing this would require
  adding new image assets or rewriting the drawing code entirely.

* Some dialogs look pretty bad, such as the certificate viewer (which has
  a tab bar with the wrong look).

* The close button placement on medium- and small-sized dialogs may be wrong;
  there are no examples in the spec.

* There are sizing/positioning issues with various dialogs (for example, in
  Javascript popups the title bar is not always long enough to hold the title).


BUG=none
TEST=none
TBR=estade@chromium.org


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

[Ben Goodger (Google), 2012-01-13 18:14:49]

Before I say LGTM, can you email me some screenshots of a few dialogs with your
changes? Include the good and the bad (e.g. the cert viewer you mentioned).

http://codereview.chromium.org/9187061/diff/1/ash/wm/dialog_frame_view.cc
File ash/wm/dialog_frame_view.cc (right):

http://codereview.chromium.org/9187061/diff/1/ash/wm/dialog_frame_view.cc#new...
ash/wm/dialog_frame_view.cc:21: // TODO(benrg): Make frame shadow and stroke
agree with the spec.
Not as a comment on this CL but for your general reference... I think we can
programmatically generate shadows. This is probably what we'll want to do going
forward so we can easily tweak shadow metrics without having to roundtrip with
the designers for new assets.

The way to do this is to use a blur mask filter and a SkDrawLooper... this is
what our Skia implementation of webkit-box-shadow does. You can look at

src/third_party/WebKit/Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp

... as an example of the skia code to write to generate a nice configurable blur
shadow.

http://codereview.chromium.org/9187061/diff/1/ash/wm/dialog_frame_view.cc#new...
ash/wm/dialog_frame_view.cc:99: // TODO(benrg): what exactly should this method
do?
I think what you have here is fine as specced. This is the views version of
WM_NCHITTEST.

[benrg, 2012-01-13 19:12:41]

http://codereview.chromium.org/9187061/diff/1/ash/wm/dialog_frame_view.cc
File ash/wm/dialog_frame_view.cc (right):

http://codereview.chromium.org/9187061/diff/1/ash/wm/dialog_frame_view.cc#new...
ash/wm/dialog_frame_view.cc:99: // TODO(benrg): what exactly should this method
do?
On 2012/01/13 18:14:50, Ben Goodger (Google) wrote:
> I think what you have here is fine as specced. This is the views version of
> WM_NCHITTEST.

I'm confused because of the other implementations.
chromeos::BubbleFrameView::NonClientHitTest returns HTNOWHERE unconditionally
(not even checking for the close box). OpaqueBrowserFrameView::NonClientHitTest
returns HTNOWHERE when the point is outside the nonclient bounds, does check for
the close box, and also calls frame()->client_view()->NonClientHitTest and
returns that value under some circumstances.

[Ben Goodger (Google), 2012-01-13 20:14:25]

The alert problem (Page at hostname says:) is a general problem with these
dialogs. You should ping that one to cole & ask for guidance.

The rest look fine for now. The tabs in the SSL one is a general issue with that
dialog regardless, and there's a separate bug logged on the fact the save file
as dialog box needs to not try to provide its own titlebar.

This change LGTM.

http://codereview.chromium.org/9187061/diff/1/ash/wm/dialog_frame_view.cc
File ash/wm/dialog_frame_view.cc (right):

http://codereview.chromium.org/9187061/diff/1/ash/wm/dialog_frame_view.cc#new...
ash/wm/dialog_frame_view.cc:99: // TODO(benrg): what exactly should this method
do?
BubbleFrameView is just wrong... OBFV is probably being OCD. I don't think
that's necessary.

[commit-bot: I haz the power, 2012-01-14 01:41:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benrg@chromium.org/9187061/10004

[commit-bot: I haz the power, 2012-01-14 01:41:26]

Presubmit check for 9187061-10004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/ui/gtk/download/download_shelf_gtk.cc,chrome/browser/resources/options2/options_page.css,chrome/browser/resources/options/options_page.css,chrome/browser/ui/gtk/custom_button.cc,chrome/browser/ui/gtk/find_bar_gtk.cc

Presubmit checks took 2.1s to calculate.

[benrg, 2012-01-14 02:21:57]

estade, I need your approval for the changes to chrome/browser/, which just
update references to assets that are being moved from chrome/app/theme/ to
ui/resources/.

[benrg, 2012-01-17 22:31:45]

I'm going to TBR this. Here are the five diffs that need review:

http://codereview.chromium.org/9187061/diff/16033/chrome/browser/resources/op...
http://codereview.chromium.org/9187061/diff/16033/chrome/browser/resources/op...
http://codereview.chromium.org/9187061/diff/16033/chrome/browser/ui/gtk/custo...
http://codereview.chromium.org/9187061/diff/16033/chrome/browser/ui/gtk/downl...
http://codereview.chromium.org/9187061/diff/16033/chrome/browser/ui/gtk/find_...

The only nontrivial change is a switch from chrome://theme/ references to path
references to the PNG images in the two CSS files. Note there were already path
references to the same images elsewhere in the same files.

[commit-bot: I haz the power, 2012-01-17 22:32:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benrg@chromium.org/9187061/16033

[commit-bot: I haz the power, 2012-01-18 04:16:32]

Can't apply patch for file
chrome/browser/ui/views/extensions/extension_installed_bubble.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/views/extensions/extension_installed_bubble.cc
Hunk #1 FAILED at 1.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/extensions/extension_installed_bubble.cc.rej

[commit-bot: I haz the power, 2012-01-18 17:24:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benrg@chromium.org/9187061/28001

[commit-bot: I haz the power, 2012-01-18 20:35:46]

Change committed as 118136

[Evan Stade, 2012-01-20 00:43:15]

http://codereview.chromium.org/9187061/diff/28001/chrome/browser/resources/op...
File chrome/browser/resources/options2/options_page.css (right):

http://codereview.chromium.org/9187061/diff/28001/chrome/browser/resources/op...
chrome/browser/resources/options2/options_page.css:161: background-image:
url('../../../../ui/resources/close_bar_p.png');
why did you do this as opposed to updating ThemeDataSource to handle the new
locations? This is extremely brittle and also makes the chrome binary larger

you also forgot to update the ntp, causing
http://code.google.com/p/chromium/issues/detail?id=110824

[Evan Stade, 2012-01-20 00:46:25]

actually, I see a lot more places where the image will be broken now: 

$ git grep "chrome://theme/IDR_CLOSE_BAR"
browser/resources/chromeos/login/user_pod_row.css:  background:
url('chrome://theme/IDR_CLOSE_BAR') center center no-repeat;
browser/resources/chromeos/login/user_pod_row.css:  background:
url('chrome://theme/IDR_CLOSE_BAR_H') center center no-repeat;
browser/resources/ntp4/new_tab.css:  background-image:
url('chrome://theme/IDR_CLOSE_BAR');
browser/resources/ntp4/new_tab.css:  background-image:
url('chrome://theme/IDR_CLOSE_BAR_H');
browser/resources/ntp4/new_tab.css:  background-image:
url('chrome://theme/IDR_CLOSE_BAR_P');
browser/resources/options/chromeos/accounts_options_page.css:  background-image:
url(chrome://theme/IDR_CLOSE_BAR);
browser/resources/options/chromeos/accounts_options_page.css:  background-image:
url(chrome://theme/IDR_CLOSE_BAR_H);
browser/resources/options2/chromeos/accounts_options_page.css: 
background-image: url(chrome://theme/IDR_CLOSE_BAR);
browser/resources/options2/chromeos/accounts_options_page.css: 
background-image: url(chrome://theme/IDR_CLOSE_BAR_H);
browser/resources/shared/css/bubble.css:  background-image:
url('chrome://theme/IDR_CLOSE_BAR');
browser/resources/shared/css/bubble.css:  background-image:
url('chrome://theme/IDR_CLOSE_BAR_H');
browser/resources/shared/css/bubble.css:  background-image:
url('chrome://theme/IDR_CLOSE_BAR_P');
browser/resources/shared/css/expandable_bubble.css:  background-image:
url('chrome://theme/IDR_CLOSE_BAR');
browser/resources/shared/css/expandable_bubble.css:  background-image:
url('chrome://theme/IDR_CLOSE_BAR_H');
browser/resources/shared/css/expandable_bubble.css:  background-image:
url('chrome://theme/IDR_CLOSE_BAR_P');

[Evan Stade, 2012-01-20 00:51:05]

for future reference, if I don't respond right away, it's better to ping me via
chat than to TBR (especially if the 3 days I fail to respond are not business
days).

[xiyuan, 2012-01-20 01:03:15]

I filed http://crbug.com/110828 for tracking chromeos login. My pending fix is
http://codereview.chromium.org/9264028/

However, the correct fix might be update ThemeSource as estade suggested.

[benrg, 2012-01-20 01:06:07]

estade, should I back this change out until it's fixed?