Fix bug 105043: Panels [WIN]: For minimize panels, taskbar hover preview show the 4-pixel represent…

Fix bug 105043: Panels [WIN]: For minimize panels, taskbar hover preview show the 4-pixel representation

BUG=105043
TEST=Manual test by minimizing panels and moving mouse over their taskbar icons


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

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

[jianli, 2012-05-21 21:34:27]

Ben, could you please my changes to NativeWidgetWin to support custom live
preview image?

Dimich, could you please review all panel related changes?

[Ben Goodger (Google), 2012-05-21 21:45:11]

http://codereview.chromium.org/10408047/diff/1/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/10408047/diff/1/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_browser_view.cc:251: SkBitmap
PanelBrowserView::GetPreviewImage(const gfx::Size& size, bool live) {
rather than add all this win32-specific code to windowdelegate, browserview
etc... why not create a new delegate interface in views that NWW uses. this code
can own or use an object that implements this interface.

[Ben Goodger (Google), 2012-05-22 23:05:48]

http://codereview.chromium.org/10408047/diff/8001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/10408047/diff/8001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:548: #if defined(OS_WIN) &&
!defined(USE_AURA)
USE_ASH

http://codereview.chromium.org/10408047/diff/8001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.h (right):

http://codereview.chromium.org/10408047/diff/8001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.h:198: #if defined(OS_WIN) &&
!defined(USE_AURA)
USE_ASH, methinks.

http://codereview.chromium.org/10408047/diff/8001/ui/base/win/hwnd_subclass.cc
File ui/base/win/hwnd_subclass.cc (right):

http://codereview.chromium.org/10408047/diff/8001/ui/base/win/hwnd_subclass.c...
ui/base/win/hwnd_subclass.cc:43: ui::SetWindowProc(target_, original_wnd_proc_);
this is actually intentionally not done. the issue is the window can be further
subclassed, so there's no safe way to unsubclass. once you've subclassed, you
have to stick around for the life of the window.

[jianli, 2012-05-22 23:34:04]

http://codereview.chromium.org/10408047/diff/8001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/10408047/diff/8001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:548: #if defined(OS_WIN) &&
!defined(USE_AURA)
On 2012/05/22 23:05:48, Ben Goodger (Google) wrote:
> USE_ASH

Done.

http://codereview.chromium.org/10408047/diff/8001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.h (right):

http://codereview.chromium.org/10408047/diff/8001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.h:198: #if defined(OS_WIN) &&
!defined(USE_AURA)
On 2012/05/22 23:05:48, Ben Goodger (Google) wrote:
> USE_ASH, methinks.

Done.

http://codereview.chromium.org/10408047/diff/8001/ui/base/win/hwnd_subclass.cc
File ui/base/win/hwnd_subclass.cc (right):

http://codereview.chromium.org/10408047/diff/8001/ui/base/win/hwnd_subclass.c...
ui/base/win/hwnd_subclass.cc:43: ui::SetWindowProc(target_, original_wnd_proc_);
On 2012/05/22 23:05:48, Ben Goodger (Google) wrote:
> this is actually intentionally not done. the issue is the window can be
further
> subclassed, so there's no safe way to unsubclass. once you've subclassed, you
> have to stick around for the life of the window.

Done.

[Ben Goodger (Google), 2012-05-22 23:41:55]

http://codereview.chromium.org/10408047/diff/6003/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/10408047/diff/6003/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:562: thumbnailer_ = new
TaskbarWindowThumbnailerWin(native_window);
keeping the thumbnailer as a member is dangerous, because it's actually owned by
the subclass. you don't need it for anything else so you can just do

thumbnail_subclass_->SetFilter(new TaskbarWindowThumbnailerWin(native_window));

[jianli, 2012-05-22 23:50:29]

http://codereview.chromium.org/10408047/diff/6003/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/10408047/diff/6003/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:562: thumbnailer_ = new
TaskbarWindowThumbnailerWin(native_window);
On 2012/05/22 23:41:55, Ben Goodger (Google) wrote:
> keeping the thumbnailer as a member is dangerous, because it's actually owned
by
> the subclass. you don't need it for anything else so you can just do
> 
> thumbnail_subclass_->SetFilter(new
TaskbarWindowThumbnailerWin(native_window));

Yes, I agree. However, I need to stop custom thumbnailing when the panel is
restored and restart custom thumbnailing  and capture a new snapshot when the
panel is minimized again.

I could move the logic to call DwmSetWindowAttribute and grab a window snapshot
to PanelBrowserView, but I need to expose the snapshot member to let
TaskbarWindowThumbnailerWin access it. This looks kind of ugly. How do you
think?

[Ben Goodger (Google), 2012-05-23 00:02:19]

LGTM

http://codereview.chromium.org/10408047/diff/6003/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/10408047/diff/6003/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:562: thumbnailer_ = new
TaskbarWindowThumbnailerWin(native_window);
Hrm. Seems like since this class owns the subclass though it can't be called
back after the subclass has deleted it, so I think your code is ok.

[Dmitry Titov, 2012-05-23 00:21:10]

lgtm

http://codereview.chromium.org/10408047/diff/6003/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/10408047/diff/6003/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:563:
thumbnail_subclass_->SetFilter(thumbnailer_);
Could you please add a short comment here explaining the thumbnailer_ is deleted
in the HWNDSubclass?

[commit-bot: I haz the power, 2012-05-23 01:07:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/10408047/2003

[commit-bot: I haz the power, 2012-05-23 01:08:10]

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

[commit-bot: I haz the power, 2012-05-23 01:16:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/10408047/13002

[commit-bot: I haz the power, 2012-05-23 01:40:06]

Try job failure for 10408047-13002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-05-23 18:09:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/10408047/4020

[commit-bot: I haz the power, 2012-05-23 20:38:32]

Change committed as 138588

[commit-bot: I haz the power, 2012-05-24 16:59:38]

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

[commit-bot: I haz the power, 2012-05-24 19:46:09]

Change committed as 138866