Animating docked background in sync with shelf

Animating docked background in sync with shelf

BUG=267582
TEST=Visual.
Dock a window.
Verify that docked background is same as the launcher background.
Open a second browser window.
Maximize it.
Verify that both docked background and the shelf background become opaque black.

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

[varkha, 2013-12-05 05:59:43]

oshima@, I am still working on this but could you please take a look? With this
CL I try to get the background of the dock and shelf to be in sync. There are
still two things I want to resolve - shelf auto-hide (which now may result in
the corner where shelf and dock meet using wrong shade - overlapping) as well as
using more than one image asset to make L-shaped appearance smooth and without
"seam". Still I wanted to see if the direction was right. Thanks!

[varkha, 2013-12-05 06:00:33]

+oshima@

[oshima, 2013-12-05 18:47:03]

https://codereview.chromium.org/98373006/diff/80001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager_observer.h (right):

https://codereview.chromium.org/98373006/diff/80001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager_observer.h:38:
internal::BackgroundAnimator::ChangeType change_type) {}
having internal class in non internal class'es method signature is odd.

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:55: const int
kTimeToSwitchBackgroundMs = 1000;
does this have to be same as the duration used in shelf background?
If so, we should have a common place to define these constants.

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:57: class
DockedWindowLayoutManager::DockedBackgroundWidget
Do we really need widget for background? Can't we simply use Layer + delegate,
like we do for shadow?

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:105:
SkBitmapOperations::ROTATION_270_CW);
does this mean we rotate the image every time we repaint?
can we update only when necessary?

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:332: shelf_layout_manager_ = NULL;
do we need to keep shelf_layout_manager_, or we can just get it from launcher_?

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.h:176: class DockedBackgroundWidget;
move this before class DockedWindowLayoutManagerObserver above.

[varkha, 2013-12-05 21:14:16]

Thanks, PTAL. I have uploaded the two images but I will probably need to ask
somebody to land those on my behalf before this CL.

https://codereview.chromium.org/98373006/diff/80001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager_observer.h (right):

https://codereview.chromium.org/98373006/diff/80001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager_observer.h:38:
internal::BackgroundAnimator::ChangeType change_type) {}
On 2013/12/05 18:47:04, oshima wrote:
> having internal class in non internal class'es method signature is odd.

Done. I have moved the enum out of internal namespace.

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:55: const int
kTimeToSwitchBackgroundMs = 1000;
On 2013/12/05 18:47:04, oshima wrote:
> does this have to be same as the duration used in shelf background?
> If so, we should have a common place to define these constants.

Done.

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:57: class
DockedWindowLayoutManager::DockedBackgroundWidget
On 2013/12/05 18:47:04, oshima wrote:
> Do we really need widget for background? Can't we simply use Layer + delegate,
like we do for shadow?

I wanted to keep the implementation in sync with the ShelfWidget. Can we revisit
those two in sync in later CL?

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:105:
SkBitmapOperations::ROTATION_270_CW);
On 2013/12/05 18:47:04, oshima wrote:
> does this mean we rotate the image every time we repaint?
> can we update only when necessary?

Done.

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:332: shelf_layout_manager_ = NULL;
On 2013/12/05 18:47:04, oshima wrote:
> do we need to keep shelf_layout_manager_, or we can just get it from
launcher_?

Done.

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.h (right):

https://codereview.chromium.org/98373006/diff/80001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.h:176: class DockedBackgroundWidget;
On 2013/12/05 18:47:04, oshima wrote:
> move this before class DockedWindowLayoutManagerObserver above.

Done.

[oshima, 2013-12-05 22:51:00]

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/background_ani...
File ash/shelf/background_animator.h (right):

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/background_ani...
ash/shelf/background_animator.h:17: CHANGE_ANIMATE,
BACKGROUND_CHANGE_ANIMATE
BACKGROUND_CHANGE_IMMEDIATE

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.h:126: const gfx::Rect& dock_bounds() { return
dock_bounds_; }
const (method)

btw, isn't work area bounds enough to draw background correctly?

[varkha, 2013-12-05 23:42:05]

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/background_ani...
File ash/shelf/background_animator.h (right):

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/background_ani...
ash/shelf/background_animator.h:17: CHANGE_ANIMATE,
On 2013/12/05 22:51:01, oshima wrote:
> BACKGROUND_CHANGE_ANIMATE
> BACKGROUND_CHANGE_IMMEDIATE
> 

Done.

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.h:126: const gfx::Rect& dock_bounds() { return
dock_bounds_; }
On 2013/12/05 22:51:01, oshima wrote:
> const (method)
> 
> btw, isn't work area bounds enough to draw background correctly?

Done. I am not sure we can rely on work address bounds with virtual keyboard.
I'd like to keep docked bounds here as it is more explicit.

[oshima, 2013-12-06 00:10:21]

lgtm

can you file a bug to optimize background drawing using layer?

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.h:126: const gfx::Rect& dock_bounds() { return
dock_bounds_; }
On 2013/12/05 23:42:05, varkha wrote:
> On 2013/12/05 22:51:01, oshima wrote:
> > const (method)
> > 
> > btw, isn't work area bounds enough to draw background correctly?
> 
> Done. I am not sure we can rely on work address bounds with virtual keyboard.
> I'd like to keep docked bounds here as it is more explicit.

I don't think virtual keyboard should have impact on work area,
as you should be able to use VK with maximized window, and showing
VK should not shrink it. Anyway, we can address that later.

[varkha, 2013-12-06 00:20:12]

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/98373006/diff/160001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.h:126: const gfx::Rect& dock_bounds() { return
dock_bounds_; }
It currently does (although the changes due to VK only affect y-coordinate).
Both docked windows and a maximized window get their bounds updated when VK is
used.

[commit-bot: I haz the power, 2013-12-06 04:14:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/98373006/210001

[commit-bot: I haz the power, 2013-12-06 13:54:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/98373006/230001

[commit-bot: I haz the power, 2013-12-06 13:55:02]

Failed to apply patch for ash/launcher/launcher_types.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/launcher/launcher_types.cc
  Hunk #1 FAILED at 8.
  1 out of 1 hunk FAILED -- saving rejects to file
ash/launcher/launcher_types.cc.rej

Patch:       ash/launcher/launcher_types.cc
Index: ash/launcher/launcher_types.cc
diff --git a/ash/launcher/launcher_types.cc b/ash/launcher/launcher_types.cc
index
46fc827ade22169a8fd3dff612e5c7f8b23fa4e2..eb806428d564d77b4daec10c085c126bfb934de8
100644
--- a/ash/launcher/launcher_types.cc
+++ b/ash/launcher/launcher_types.cc
@@ -8,6 +8,7 @@ namespace ash {
 
 const int kLauncherPreferredSize = 48;
 const int kLauncherBackgroundAlpha = 204;
+const int kTimeToSwitchBackgroundMs = 1000;
 
 LauncherItem::LauncherItem()
     : type(TYPE_UNDEFINED),

[commit-bot: I haz the power, 2013-12-06 15:14:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/98373006/270001

[commit-bot: I haz the power, 2013-12-06 15:48:14]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-06 16:22:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/98373006/270001

[commit-bot: I haz the power, 2013-12-06 16:44:49]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-06 19:37:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/98373006/270001

[commit-bot: I haz the power, 2013-12-07 01:00:15]

Change committed as 239284