Make the dismiss button on the CF infobar be an image button.

Make the dismiss button on the CF infobar be an image button.

Uses WTL's CBitmapButton class, but applies a bug fix and a workaround
to get CBitmapButton to both correctly erase the background and resize
correctly when hosted in a dialog managed by CDialogResize.

Also uses a correctly formatted BMP to store the button state list,
if editing that BMP in the future, make sure that the EXACT RGBA format
and bitness are preserved or things will break.

Lastly, fxies a layout issue with the "more info" link vanishing during resizing.

BUG=263809
TEST=NONE

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

[robertshield, 2013-07-11 18:23:29]

Let's have a bitmap button he says. It'll be easy he says.

In any case: victory! PTAL.

[grt (UTC plus 2), 2013-07-22 16:07:27]

hacking around WTL. awesome.

https://codereview.chromium.org/18769007/diff/1/chrome/app/cf_resources.rc
File chrome/app/cf_resources.rc (right):

https://codereview.chromium.org/18769007/diff/1/chrome/app/cf_resources.rc#ne...
chrome/app/cf_resources.rc:54: IDD_CHROME_FRAME_TURNDOWN_PROMPT DIALOGEX 0, 0,
393, 14
the dialog is 14px tall, yet IDDISMISS is 18. how is this correct?

https://codereview.chromium.org/18769007/diff/1/chrome/app/cf_resources.rc#ne...
chrome/app/cf_resources.rc:60: PUSHBUTTON     
"Uninstall",IDUNINSTALL,318,2,50,12
since your change shrinks IDDISMISS from a width of 50 down to 18, i would have
expected IDUNINSTALL to now be at 317 and for IDDISMISS to be at 372. did the
spacing between the two need to change for it to look nice?

https://codereview.chromium.org/18769007/diff/1/chrome/app/cf_resources.rc#ne...
chrome/app/cf_resources.rc:61: DEFPUSHBUTTON   "Dismiss",IDDISMISS,378,2,18,18
378 + 18 = 396, which is greater than the width of the dialog (393). why so?

https://codereview.chromium.org/18769007/diff/1/chrome/app/cf_resources.rc#ne...
chrome/app/cf_resources.rc:63: 
it looks like the cf_resources.rc in my checkout already ends with the right
number of newlines. does your patch add a new one?

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
File chrome_frame/turndown_prompt/turndown_prompt_window.cc (right):

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:38: CFBitmapButton() :
  CFBitmapButton()
      : CBitmapButtonImpl<CFBitmapButton>(BMPBTN_AUTOSIZE | BMPBTN_HOVER,
                                          NULL) {}

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:46: dc.FillRect(&rc,
(HBRUSH)(COLOR_BTNFACE+1));
reinterpret_cast<HBRUSH>(COLOR_BTNFACE + 1)

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:93: const
base::WeakPtr<TurndownPromptWindow>& instance) {
since this function doesn't store |instance| for later use, i think it's more
clear to just pass a naked TurndownPromptWindow*.

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:112: ILC_COLOR8|ILC_MASK,
4, 0);
ILC_COLOR8 | ILC_MASK

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:113:
instance->close_button_->m_ImageList.Add(close_bitmap, RGB(255,0,255));
255, 0, 255

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:117:
instance->close_button_->SetImages(0, 1, 2, 3);
4, 5, 6, 7, 8
who do we appreciate
robertshield
robertshield
yay!

[robertshield, 2013-07-22 19:58:57]

Thanks, PTAL.

https://codereview.chromium.org/18769007/diff/1/chrome/app/cf_resources.rc
File chrome/app/cf_resources.rc (right):

https://codereview.chromium.org/18769007/diff/1/chrome/app/cf_resources.rc#ne...
chrome/app/cf_resources.rc:54: IDD_CHROME_FRAME_TURNDOWN_PROMPT DIALOGEX 0, 0,
393, 14
On 2013/07/22 16:07:27, grt wrote:
> the dialog is 14px tall, yet IDDISMISS is 18. how is this correct?

It may be specified as 14 px tall, but it renders as 22 px or so.

https://codereview.chromium.org/18769007/diff/1/chrome/app/cf_resources.rc#ne...
chrome/app/cf_resources.rc:60: PUSHBUTTON     
"Uninstall",IDUNINSTALL,318,2,50,12
On 2013/07/22 16:07:27, grt wrote:
> since your change shrinks IDDISMISS from a width of 50 down to 18, i would
have
> expected IDUNINSTALL to now be at 317 and for IDDISMISS to be at 372. did the
> spacing between the two need to change for it to look nice?

The spacing is slightly larger to line up with the right edge of buttons on any
chrome infobar appearing below. Tweaked slightly to get them to line up
perfectly.

https://codereview.chromium.org/18769007/diff/1/chrome/app/cf_resources.rc#ne...
chrome/app/cf_resources.rc:61: DEFPUSHBUTTON   "Dismiss",IDDISMISS,378,2,18,18
On 2013/07/22 16:07:27, grt wrote:
> 378 + 18 = 396, which is greater than the width of the dialog (393). why so?

This was probably an error, although it was masked by the re-layouting code.
Changed to 373, which should provide a 2 px offset from the right of the dialog.

https://codereview.chromium.org/18769007/diff/1/chrome/app/cf_resources.rc#ne...
chrome/app/cf_resources.rc:63: 
On 2013/07/22 16:07:27, grt wrote:
> it looks like the cf_resources.rc in my checkout already ends with the right
> number of newlines. does your patch add a new one?

Done.

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
File chrome_frame/turndown_prompt/turndown_prompt_window.cc (right):

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:38: CFBitmapButton() :
On 2013/07/22 16:07:27, grt wrote:
>   CFBitmapButton()
>       : CBitmapButtonImpl<CFBitmapButton>(BMPBTN_AUTOSIZE | BMPBTN_HOVER,
>                                           NULL) {}

Done.

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:46: dc.FillRect(&rc,
(HBRUSH)(COLOR_BTNFACE+1));
On 2013/07/22 16:07:27, grt wrote:
> reinterpret_cast<HBRUSH>(COLOR_BTNFACE + 1)

Done.

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:93: const
base::WeakPtr<TurndownPromptWindow>& instance) {
On 2013/07/22 16:07:27, grt wrote:
> since this function doesn't store |instance| for later use, i think it's more
> clear to just pass a naked TurndownPromptWindow*.

Done.

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:112: ILC_COLOR8|ILC_MASK,
4, 0);
On 2013/07/22 16:07:27, grt wrote:
> ILC_COLOR8 | ILC_MASK

Done.

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:113:
instance->close_button_->m_ImageList.Add(close_bitmap, RGB(255,0,255));
On 2013/07/22 16:07:27, grt wrote:
> 255, 0, 255

Done.

https://codereview.chromium.org/18769007/diff/1/chrome_frame/turndown_prompt/...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:117:
instance->close_button_->SetImages(0, 1, 2, 3);
On 2013/07/22 16:07:27, grt wrote:
> 4, 5, 6, 7, 8
> who do we appreciate
> robertshield
> robertshield
> yay!

\o/

[grt (UTC plus 2), 2013-07-23 01:38:33]

nice work, my man. lgtm with one code motion nit.

https://codereview.chromium.org/18769007/diff/7001/chrome_frame/turndown_prom...
File chrome_frame/turndown_prompt/turndown_prompt_window.cc (right):

https://codereview.chromium.org/18769007/diff/7001/chrome_frame/turndown_prom...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:93: void
TurndownPromptWindow::SetupBitmapButton(TurndownPromptWindow* instance) {
oops. i forgot to mention in my previous review that this should immediately
follow the definition of ~TurndownPromptWindow to maintain the method ordering
in the header file. please move it down there. thanks.

https://codereview.chromium.org/18769007/diff/7001/chrome_frame/turndown_prom...
File chrome_frame/turndown_prompt/turndown_prompt_window.h (right):

https://codereview.chromium.org/18769007/diff/7001/chrome_frame/turndown_prom...
chrome_frame/turndown_prompt/turndown_prompt_window.h:74:
DLGRESIZE_CONTROL(IDC_TD_PROMPT_LINK, DLSZ_CENTER_Y | DLSZ_MOVE_X)
is this the change that fixed the flickering?

[robertshield, 2013-07-23 03:09:57]

Thanks!

https://codereview.chromium.org/18769007/diff/7001/chrome_frame/turndown_prom...
File chrome_frame/turndown_prompt/turndown_prompt_window.cc (right):

https://codereview.chromium.org/18769007/diff/7001/chrome_frame/turndown_prom...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:93: void
TurndownPromptWindow::SetupBitmapButton(TurndownPromptWindow* instance) {
On 2013/07/23 01:38:33, grt wrote:
> oops. i forgot to mention in my previous review that this should immediately
> follow the definition of ~TurndownPromptWindow to maintain the method ordering
> in the header file. please move it down there. thanks.

Done.

https://codereview.chromium.org/18769007/diff/7001/chrome_frame/turndown_prom...
File chrome_frame/turndown_prompt/turndown_prompt_window.h (right):

https://codereview.chromium.org/18769007/diff/7001/chrome_frame/turndown_prom...
chrome_frame/turndown_prompt/turndown_prompt_window.h:74:
DLGRESIZE_CONTROL(IDC_TD_PROMPT_LINK, DLSZ_CENTER_Y | DLSZ_MOVE_X)
On 2013/07/23 01:38:33, grt wrote:
> is this the change that fixed the flickering?

No, it was reducing the width so they didn't overlap that did it. This does list
the controls in right to left order which is slightly better than the randomish
order before imo, so I'll leave it unless you erupt in violent boils at the
sight of it.

[robertshield, 2013-07-23 03:10:41]

+Carlos for chrome/app OWNERS

[grt (UTC plus 2), 2013-07-23 14:06:05]

In case you haven't already noticed: it looks like you'll need to dcommit the
binary file before you can send the rest of this through the CQ.

[robertshield, 2013-07-23 14:17:59]

On 2013/07/23 14:06:05, grt wrote:
> In case you haven't already noticed: it looks like you'll need to dcommit the
> binary file before you can send the rest of this through the CQ.

Thanks, yes, I had noticed. I'll wait for Carlos to take a look first though.

[cpu_(ooo_6.6-7.5), 2013-07-24 02:05:08]

lgtm

https://codereview.chromium.org/18769007/diff/15001/chrome_frame/turndown_pro...
File chrome_frame/turndown_prompt/turndown_prompt_window.cc (right):

https://codereview.chromium.org/18769007/diff/15001/chrome_frame/turndown_pro...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:45: RECT rc;
lets rc = {0} or check error on the Get. I vote for {0}

https://codereview.chromium.org/18769007/diff/15001/chrome_frame/turndown_pro...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:126:
instance->close_button_->m_ImageList.Add(close_bitmap, RGB(255, 0, 255));
does the imagelist own the close_bitmap? I forgot what the ownership model is
here. I see the .Destroy() on line 156.

[robertshield, 2013-07-24 14:02:54]

Thanks!

I'll break the bitmap out into a dcomitted CL then run this one through the try
bots before committing.

https://codereview.chromium.org/18769007/diff/15001/chrome_frame/turndown_pro...
File chrome_frame/turndown_prompt/turndown_prompt_window.cc (right):

https://codereview.chromium.org/18769007/diff/15001/chrome_frame/turndown_pro...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:45: RECT rc;
On 2013/07/24 02:05:09, cpu wrote:
> lets rc = {0} or check error on the Get. I vote for {0}

Done.

https://codereview.chromium.org/18769007/diff/15001/chrome_frame/turndown_pro...
chrome_frame/turndown_prompt/turndown_prompt_window.cc:126:
instance->close_button_->m_ImageList.Add(close_bitmap, RGB(255, 0, 255));
On 2013/07/24 02:05:09, cpu wrote:
> does the imagelist own the close_bitmap? I forgot what the ownership model is
> here. I see the .Destroy() on line 156.

I thought it did, but indeed it doesn't. Thanks for checking, now freeing it.

[commit-bot: I haz the power, 2013-07-24 18:46:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/18769007/29001

[commit-bot: I haz the power, 2013-07-24 21:48:15]

Change committed as 213521