While dragging a window, show a semi-transparent aura window instead of the standard gray phantom wi

While dragging a window, show a semi-transparent aura window instead of the standard gray phantom window.

BUG=136816
TEST=new unit tests passed on try


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

[Yusuke Sato, 2012-08-09 15:22:22]

Scott, this is the 4th (and the final) CL for window dragging. Could you have a
look?

[sky, 2012-08-09 16:23:16]

http://codereview.chromium.org/10823199/diff/7009/ash/wm/workspace/phantom_wi...
File ash/wm/workspace/phantom_window_controller.cc (right):

http://codereview.chromium.org/10823199/diff/7009/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:112: aura::RootWindow*
root_window,
I would rather see a set_style and SetDestinationDisplay then these new args.

http://codereview.chromium.org/10823199/diff/7009/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:162: ui::Layer* layer =
phantom_widget_->GetNativeWindow()->layer();
phantom_widget_ may be NULL.

http://codereview.chromium.org/10823199/diff/7009/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:168: return
phantom_widget_->GetNativeWindow()->layer()->opacity();
phanton_widget_ may be NULL.

http://codereview.chromium.org/10823199/diff/7009/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:185: params.type =
views::Widget::InitParams::TYPE_WINDOW;
Why does this need to be a WINDOW?

[Yusuke Sato, 2012-08-09 20:00:17]

Please take another look.

https://chromiumcodereview.appspot.com/10823199/diff/7009/ash/wm/workspace/ph...
File ash/wm/workspace/phantom_window_controller.cc (right):

https://chromiumcodereview.appspot.com/10823199/diff/7009/ash/wm/workspace/ph...
ash/wm/workspace/phantom_window_controller.cc:112: aura::RootWindow*
root_window,
On 2012/08/09 16:23:16, sky wrote:
> I would rather see a set_style and SetDestinationDisplay then these new args.

Done.

https://chromiumcodereview.appspot.com/10823199/diff/7009/ash/wm/workspace/ph...
ash/wm/workspace/phantom_window_controller.cc:162: ui::Layer* layer =
phantom_widget_->GetNativeWindow()->layer();
On 2012/08/09 16:23:16, sky wrote:
> phantom_widget_ may be NULL.

Thanks. Done.

https://chromiumcodereview.appspot.com/10823199/diff/7009/ash/wm/workspace/ph...
ash/wm/workspace/phantom_window_controller.cc:168: return
phantom_widget_->GetNativeWindow()->layer()->opacity();
On 2012/08/09 16:23:16, sky wrote:
> phanton_widget_ may be NULL.

Done.

https://chromiumcodereview.appspot.com/10823199/diff/7009/ash/wm/workspace/ph...
ash/wm/workspace/phantom_window_controller.cc:185: params.type =
views::Widget::InitParams::TYPE_WINDOW;
On 2012/08/09 16:23:16, sky wrote:
> Why does this need to be a WINDOW?

POPUP is fine. Reverted this part. Thanks.

[sky, 2012-08-09 21:35:28]

Doesn't look like your diff made it up correctly. Could you try again?

[Yusuke Sato, 2012-08-09 21:39:30]

On 2012/08/09 21:35:28, sky wrote:
> Doesn't look like your diff made it up correctly. Could you try again?

re-uploaded.

[sky, 2012-08-09 21:57:27]

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/phantom_wi...
File ash/wm/workspace/phantom_window_controller.cc (right):

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:92: virtual void
Paint(gfx::Canvas* canvas, const gfx::Size& size) OVERRIDE {
This won't include any child windows the window may have.

I think you need to make a copy of the layer tree like some of the animation
code does.

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:166: if (!phantom_widget_)
I don't like that set_style and SetOpacity have different semantics. Make them
the same.

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer.h (right):

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.h:55: PhantomWindowController*
snap_phantom_window_controller_for_testing() const {
Instead of this, make the test a friend. Or create a class that is only compiled
during tests that is a friend.

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.h:126: void
UpdateDragPhantomWindow(const gfx::Rect& bounds, bool in_original_root);
Document |in_original_root|.

[Yusuke Sato, 2012-08-10 00:30:49]

Thanks. Please take another look.

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/phantom_wi...
File ash/wm/workspace/phantom_window_controller.cc (right):

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:92: virtual void
Paint(gfx::Canvas* canvas, const gfx::Size& size) OVERRIDE {
On 2012/08/09 21:57:27, sky wrote:
> This won't include any child windows the window may have.
> 
> I think you need to make a copy of the layer tree like some of the animation
> code does.

Would you mind if I implement this in another CL? I read the animation code like
ash/wm/window_animations.cc a few days ago, but couldn't figure out a good way
to let another root window use a layer returned by Window::RecreateLayer(). I've
filed a bug and added a TODO to this CL.

UX team also told me that for the first version it's okay not to draw a web
content area during dragging.

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:166: if (!phantom_widget_)
On 2012/08/09 21:57:27, sky wrote:
> I don't like that set_style and SetOpacity have different semantics. Make them
> the same.

Done.

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer.h (right):

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.h:55: PhantomWindowController*
snap_phantom_window_controller_for_testing() const {
On 2012/08/09 21:57:27, sky wrote:
> Instead of this, make the test a friend. Or create a class that is only
compiled
> during tests that is a friend.

Done.

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.h:126: void
UpdateDragPhantomWindow(const gfx::Rect& bounds, bool in_original_root);
On 2012/08/09 21:57:27, sky wrote:
> Document |in_original_root|.

Done.

[sky, 2012-08-10 16:06:36]

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/phantom_wi...
File ash/wm/workspace/phantom_window_controller.cc (right):

http://codereview.chromium.org/10823199/diff/4016/ash/wm/workspace/phantom_wi...
ash/wm/workspace/phantom_window_controller.cc:92: virtual void
Paint(gfx::Canvas* canvas, const gfx::Size& size) OVERRIDE {
On 2012/08/10 00:30:49, Yusuke Sato wrote:
> On 2012/08/09 21:57:27, sky wrote:
> > This won't include any child windows the window may have.
> > 
> > I think you need to make a copy of the layer tree like some of the animation
> > code does.
> 
> Would you mind if I implement this in another CL? I read the animation code
like
> ash/wm/window_animations.cc a few days ago, but couldn't figure out a good way
> to let another root window use a layer returned by Window::RecreateLayer().
I've
> filed a bug and added a TODO to this CL.
> 
> UX team also told me that for the first version it's okay not to draw a web
> content area during dragging.

If you make this paint an empty square, I'm ok with a TODO.

[sky, 2012-08-10 16:14:48]

LGTM

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/10823199/7012

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

Change committed as 151073