Update ConstrainedWindowViews appearance according to mock

This CL supersedes previous ConstrainedWindow Views CLs and contains much of the work required to update
ConstrainedWindowViews to meet the mock specified at:
http://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup2/dialogs_markup2g.html

I believe that all the features of the mock are implemented by this CL,
except:
 - dialog shadow
 - dialog animations
 - button shadows
 - some aspects of button text layout

Note that this CL attempts to fully address *only* the repost form
warning dialog.  The intention is to complete the remaining work
(features and dialogs) in future CLs.  The ConstrainedWindow changes are
behind a flag to allow for incremental progress.

The first patch set contains changes to position the constrained window
so that it overlaps the browser chrome.

BUG=140517, 140537, 140539, 140554, 140562
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159994

[Mike Wittman, 2012-09-14 01:15:29]

Hi Ben,

Please take a look at this new review for the ConstrainedWindow updates to
Views.  I believe this should now be sufficiently comprehensive to understand
the ConstrainedWindow changes as a whole; it contains the changes necessary to
update a sample dialog (the repost form warning dialog) to be substantially
consistent with the mock.  If not, please let me know what else you would like
to see.

Thanks!

[Ben Goodger (Google), 2012-09-17 22:37:26]

https://codereview.chromium.org/10933085/diff/10026/ui/views/controls/button/...
File ui/views/controls/button/web_style_text_button.cc (right):

https://codereview.chromium.org/10933085/diff/10026/ui/views/controls/button/...
ui/views/controls/button/web_style_text_button.cc:55: class
WebStyleTextButtonBackground : public Background {
Also I would like to come up with a different name for this stuff. WebStyle had
me confused for a while. ChromeStyle?

https://codereview.chromium.org/10933085/diff/10026/ui/views/controls/button/...
File ui/views/controls/button/web_style_text_button.h (right):

https://codereview.chromium.org/10933085/diff/10026/ui/views/controls/button/...
ui/views/controls/button/web_style_text_button.h:24: class VIEWS_EXPORT
WebStyleTextButton : public TextButton {
Can we avoid adding Yet-Another-Button-Subclass and instead do these
customizations as:

- the background/border etc stuff that you have in the .cc file
- improvements to the TextButton.

https://codereview.chromium.org/10933085/diff/10026/ui/views/window/dialog_cl...
File ui/views/window/dialog_client_view.h (right):

https://codereview.chromium.org/10933085/diff/10026/ui/views/window/dialog_cl...
ui/views/window/dialog_client_view.h:39: enum Style {
You should not need to add this.

https://codereview.chromium.org/10933085/diff/10026/ui/views/window/dialog_cl...
ui/views/window/dialog_client_view.h:62: TextButton* ok_button() const { return
ok_button_; }
This is OK.

[Mike Wittman, 2012-09-21 22:53:17]

Hi Ben, please take another look when you have the chance.

http://codereview.chromium.org/10933085/diff/10026/ui/views/controls/button/w...
File ui/views/controls/button/web_style_text_button.cc (right):

http://codereview.chromium.org/10933085/diff/10026/ui/views/controls/button/w...
ui/views/controls/button/web_style_text_button.cc:55: class
WebStyleTextButtonBackground : public Background {
On 2012/09/17 22:37:26, Ben Goodger (Google) wrote:
> Also I would like to come up with a different name for this stuff. WebStyle
had
> me confused for a while. ChromeStyle?

Works for me.  I wasn't sure what the best name for this was.

http://codereview.chromium.org/10933085/diff/10026/ui/views/controls/button/w...
File ui/views/controls/button/web_style_text_button.h (right):

http://codereview.chromium.org/10933085/diff/10026/ui/views/controls/button/w...
ui/views/controls/button/web_style_text_button.h:24: class VIEWS_EXPORT
WebStyleTextButton : public TextButton {
On 2012/09/17 22:37:26, Ben Goodger (Google) wrote:
> Can we avoid adding Yet-Another-Button-Subclass and instead do these
> customizations as:
> 
> - the background/border etc stuff that you have in the .cc file
> - improvements to the TextButton.

Yes.  I've refactored the necessary functionality into button base classes.

http://codereview.chromium.org/10933085/diff/10026/ui/views/window/dialog_cli...
File ui/views/window/dialog_client_view.h (right):

http://codereview.chromium.org/10933085/diff/10026/ui/views/window/dialog_cli...
ui/views/window/dialog_client_view.h:39: enum Style {
On 2012/09/17 22:37:26, Ben Goodger (Google) wrote:
> You should not need to add this.

I've removed the enum in favor of accepting StyleParams via the constructor.

[Ben Goodger (Google), 2012-09-27 16:16:12]

Getting there. Can you also email me screenshots.

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/constrai...
File chrome/browser/ui/constrained_window_tab_helper_delegate.cc (right):

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/constrai...
chrome/browser/ui/constrained_window_tab_helper_delegate.cc:22: {
move to prev line

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_frame_simple.cc (right):

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.cc:199: switch
(sender->tag()) {
then in here you can do:

DCHECK_EQ(close_button_, sender);
sender->GetWidget()->Close();

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_frame_simple.h (right):

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.h:32: CLOSE_WIDGET
rather than use tag (which we are moving away from) just store a View* on this
guy that is the close button... (see next file)

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_views.cc (right):

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_views.cc:637: SetBounds(bounds);
rather than creating at the wrong location the moving, can you pass the correct
bounds to Init via params?

http://codereview.chromium.org/10933085/diff/13001/ui/views/controls/button/c...
File ui/views/controls/button/chrome_style.cc (right):

http://codereview.chromium.org/10933085/diff/13001/ui/views/controls/button/c...
ui/views/controls/button/chrome_style.cc:220: case CustomButton::BS_NORMAL:
outdent the whole body of the switch by 2 spaces, here and elsewhere in this cl

[Mike Wittman, 2012-09-28 22:46:09]

Screenshots sent separately.

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/constrai...
File chrome/browser/ui/constrained_window_tab_helper_delegate.cc (right):

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/constrai...
chrome/browser/ui/constrained_window_tab_helper_delegate.cc:22: {
On 2012/09/27 16:16:12, Ben Goodger (Google) wrote:
> move to prev line

Done.

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_frame_simple.cc (right):

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.cc:199: switch
(sender->tag()) {
On 2012/09/27 16:16:12, Ben Goodger (Google) wrote:
> then in here you can do:
> 
> DCHECK_EQ(close_button_, sender);
> sender->GetWidget()->Close();

This window allows a configurable header View (to be used by Web Intents), so
I've made this a conditional rather than DCHECK and allowed the user to specify
a close button along with the header View.

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_frame_simple.h (right):

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_frame_simple.h:32: CLOSE_WIDGET
On 2012/09/27 16:16:12, Ben Goodger (Google) wrote:
> rather than use tag (which we are moving away from) just store a View* on this
> guy that is the close button... (see next file)

Done.

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_views.cc (right):

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_views.cc:637: SetBounds(bounds);
On 2012/09/27 16:16:12, Ben Goodger (Google) wrote:
> rather than creating at the wrong location the moving, can you pass the
correct
> bounds to Init via params?

Not easily.  The window centering logic lives within platform-specific code and
wants to operate on both dimensions of a pre-existing window.  It's probably
possible to plumb custom calls down there to compute horizontal-only centering
for a window that doesn't yet exist, but it would add a lot of complexity to
support a situation that's likely a one-off.

[Ben Goodger (Google), 2012-10-01 16:25:57]

From your screenshot, I'd like to see some kind of 1px stroke on this panel if
you can't achieve any kind of shadow.

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_views.cc (right):

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_views.cc:637: SetBounds(bounds);
But you can construct correct bounds for params earlier in this function, e.g.
near where you set parent and before calling Init()

[Mike Wittman, 2012-10-02 00:07:35]

Can we address the shadow/stroke in a future CL, when we address the remaining
UI polish for the constrained window?  We'd like to land the pieces of this CL
that affect Web Intents, so they can start coding against the new style dialog
for M24.

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
File chrome/browser/ui/views/constrained_window_views.cc (right):

http://codereview.chromium.org/10933085/diff/13001/chrome/browser/ui/views/co...
chrome/browser/ui/views/constrained_window_views.cc:637: SetBounds(bounds);
No, on further inspection I don't see a feasible way to do this.  We need to
know the width of the widget to center it horizontally over the browser window. 
But the width is dependent on the widths of the client and non-client views,
neither of which are created until Init().

[Ben Goodger (Google), 2012-10-02 16:26:31]

lgtm

[commit-bot: I haz the power, 2012-10-02 17:02:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/10933085/33002

[commit-bot: I haz the power, 2012-10-02 17:02:17]

Failed to apply patch for
chrome/browser/ui/views/constrained_window_frame_simple.h:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/views/constrained_window_frame_simple.h
Hunk #2 FAILED at 39.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/constrained_window_frame_simple.h.rej

[commit-bot: I haz the power, 2012-10-02 21:16:03]

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

[commit-bot: I haz the power, 2012-10-02 22:06:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/10933085/43002

[commit-bot: I haz the power, 2012-10-02 23:16:43]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-03 15:25:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/10933085/32008

[commit-bot: I haz the power, 2012-10-03 15:25:46]

Failed to apply patch for
chrome/browser/ui/constrained_window_tab_helper_delegate.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/constrained_window_tab_helper_delegate.cc
Hunk #1 FAILED at 4.
Hunk #2 succeeded at 13 (offset -4 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/constrained_window_tab_helper_delegate.cc.rej

[commit-bot: I haz the power, 2012-10-03 17:20:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/10933085/42026

[commit-bot: I haz the power, 2012-10-03 20:08:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/10933085/46077

[commit-bot: I haz the power, 2012-10-03 22:33:21]

Change committed as 159994

[hshi1, 2012-10-04 02:29:34]

https://chromiumcodereview.appspot.com/10933085/diff/46077/ui/views/controls/...
File ui/views/controls/button/text_button.cc (right):

https://chromiumcodereview.appspot.com/10933085/diff/46077/ui/views/controls/...
ui/views/controls/button/text_button.cc:735:
prefsize.set_width(std::max(prefsize.height(), min_width_));
Shouldn't the above be prefsize.set_width(std::max(prefsize.width(),
min_width_)); ??