Make browser windows override_redirect when dragging tabs.

Make browser windows override_redirect when dragging tabs.

BUG=416119

[stapelberg, 2015-11-18 14:20:13]

Description was changed from

==========
Make browser windows override_redirect when dragging tabs.

BUG=416119
==========

to

==========
Make browser windows override_redirect when dragging tabs.

BUG=416119
==========

[stapelberg, 2015-11-18 14:20:13]

stapelberg@google.com changed reviewers:
+ derat@chromium.org

[stapelberg, 2015-11-18 14:20:40]

This is a first draft to get some feedback on how I should restructure what this
change is doing.

[Daniel Erat, 2015-11-18 16:16:46]

derat@chromium.org changed reviewers:
+ ben@chromium.org, erg@chromium.org

[Daniel Erat, 2015-11-18 16:16:47]

+ben and elliot for their thoughts on this.

http://crbug.com/416119#c12 is probably a good summary.

https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:197: bool override_redirect;
this should be desktop-linux-only. i _think_ that the way to detect that is:

  #if !defined(OS_CHROMEOS) && defined(USE_X11)

but i'm also wondering if is_dragged_tab or something similar would be more
appropriate. are there any cases where we'd have an override-redirect browser
otherwise?

https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_frame.cc:72: params.type =
views::Widget::InitParams::TYPE_DRAG_BROWSER;
it seems weird to infer this type based on the override-redirect-ness of the
browser. is there some other way to detect this?

[Ben Goodger (Google), 2015-11-18 16:28:53]

Would like to avoid adding platform-specific methods like this to
WindowTreeHost.

On Wed, Nov 18, 2015 at 8:16 AM, <derat@chromium.org> wrote:

> +ben and elliot for their thoughts on this.
>
> http://crbug.com/416119#c12 is probably a good summary.
>
>
>
> https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/browser.h
> File chrome/browser/ui/browser.h (right):
>
>
>
https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/browser.h...
> chrome/browser/ui/browser.h:197: bool override_redirect;
> this should be desktop-linux-only. i _think_ that the way to detect that
> is:
>
>   #if !defined(OS_CHROMEOS) && defined(USE_X11)
>
> but i'm also wondering if is_dragged_tab or something similar would be
> more appropriate. are there any cases where we'd have an
> override-redirect browser otherwise?
>
>
>
https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/views/fra...
> File chrome/browser/ui/views/frame/browser_frame.cc (right):
>
>
>
https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/views/fra...
> chrome/browser/ui/views/frame/browser_frame.cc:72: params.type =
> views::Widget::InitParams::TYPE_DRAG_BROWSER;
> it seems weird to infer this type based on the override-redirect-ness of
> the browser. is there some other way to detect this?
>
> https://codereview.chromium.org/1456963002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stapelberg, 2015-11-18 16:34:08]

On 2015/11/18 16:28:53, Ben Goodger (Google) wrote:
> Would like to avoid adding platform-specific methods like this to
> WindowTreeHost.

What alternative do you suggest?

[Elliot Glaysher, 2015-11-18 18:20:21]

https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:197: bool override_redirect;
On 2015/11/18 16:16:47, Daniel Erat wrote:
> this should be desktop-linux-only. i _think_ that the way to detect that is:
> 
>   #if !defined(OS_CHROMEOS) && defined(USE_X11)
> 
> but i'm also wondering if is_dragged_tab or something similar would be more
> appropriate. are there any cases where we'd have an override-redirect browser
> otherwise?

It's:

#if defined(OS_LINUX) && !defined(OS_CHROMEOS)

https://codereview.chromium.org/1456963002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1456963002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1291:
XMapWindow(xdisplay_, xwindow_);
Have you tested this in other window managers? This seems like the sort of state
that could be improperly cached, and I vaguely remember a bug like that in the
handling of override redirect.

[Daniel Erat, 2015-11-18 18:38:36]

https://codereview.chromium.org/1456963002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1456963002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1291:
XMapWindow(xdisplay_, xwindow_);
On 2015/11/18 18:20:21, Elliot Glaysher wrote:
> Have you tested this in other window managers? This seems like the sort of
state
> that could be improperly cached, and I vaguely remember a bug like that in the
> handling of override redirect.

oh, yeah. i was recommending creating a new window and reparenting the contents
into it to avoid this.

i think it depends on the window manager -- some may create an internal object
to track a window when it's first created, while others may wait until it's
first mapped. i wouldn't trust any of them to do something reasonable if the
override-redirect state of a window that they're already tracking changes. :-P

(it's almost as if the people who wrote ICCCM and EWMH should've also written
exhaustive tests so WM developers could verify that their code behaves
correctly! :-P)

[stapelberg, 2015-12-23 13:14:34]

On 2015/11/18 16:34:08, stapelberg wrote:
> On 2015/11/18 16:28:53, Ben Goodger (Google) wrote:
> > Would like to avoid adding platform-specific methods like this to
> > WindowTreeHost.
> 
> What alternative do you suggest?

Ben, I’m still unclear on what the alternative to adding this method is, given
that we don’t use RTTI in Chromium and given that I need to call this method
from TabDragController::CompleteDrag(). Can you elaborate please?

[stapelberg, 2015-12-23 13:17:24]

Sorry for letting this sit so long.

https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:197: bool override_redirect;
On 2015/11/18 18:20:21, Elliot Glaysher wrote:
> On 2015/11/18 16:16:47, Daniel Erat wrote:
> > this should be desktop-linux-only. i _think_ that the way to detect that is:
> > 
> >   #if !defined(OS_CHROMEOS) && defined(USE_X11)
> > 
> > but i'm also wondering if is_dragged_tab or something similar would be more
> > appropriate. are there any cases where we'd have an override-redirect
browser
> > otherwise?
> 
> It's:
> 
> #if defined(OS_LINUX) && !defined(OS_CHROMEOS)

Done.

https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/1456963002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_frame.cc:72: params.type =
views::Widget::InitParams::TYPE_DRAG_BROWSER;
On 2015/11/18 16:16:47, Daniel Erat wrote:
> it seems weird to infer this type based on the override-redirect-ness of the
> browser. is there some other way to detect this?

Well, given that TYPE_DRAG_BROWSER is exactly TYPE_BROWSER, but with the
difference of using an undecorated frame (override-redirect), I think it’s
appropriate.

How do you suggest to solve your concern? To me, it seems like renaming either
of the two properties might be a good solution. I’m willing to do that, but I
don’t have a good suggestion. What are your thoughts?

https://codereview.chromium.org/1456963002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1456963002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1291:
XMapWindow(xdisplay_, xwindow_);
On 2015/11/18 18:38:36, Daniel Erat wrote:
> On 2015/11/18 18:20:21, Elliot Glaysher wrote:
> > Have you tested this in other window managers? This seems like the sort of
> state
> > that could be improperly cached, and I vaguely remember a bug like that in
the
> > handling of override redirect.
> 
> oh, yeah. i was recommending creating a new window and reparenting the
contents
> into it to avoid this.
> 
> i think it depends on the window manager -- some may create an internal object
> to track a window when it's first created, while others may wait until it's
> first mapped. i wouldn't trust any of them to do something reasonable if the
> override-redirect state of a window that they're already tracking changes. :-P
> 
> (it's almost as if the people who wrote ICCCM and EWMH should've also written
> exhaustive tests so WM developers could verify that their code behaves
> correctly! :-P)

Not entirely sure what you mean by “other window managers”, do you have a list
of window managers in which you’d like me to test this code?

Note that we’re creating the window with override-redirect=True, meaning it will
override any redirects, including the MapWindow redirect on the root window, so
window managers will not even be informed that this window exists. I’d be very
surprised to find out that some window manager felt the need to come up with a
non-standard way of figuring out when new windows are created (repeatedly
querying the tree?) and then tracking the override-redirect state and not
updating it.

I’ve considered doing the reparenting regardless, but it actually blows up the
code a lot for two reasons:

1. AFAICT, there is no way to identify the content window (the GL surface
window) from within the code itself, because it is created and maintained by the
GPU process. Hence, I’d need to use XQueryTree() to find all child windows of
xwindow_.

2. I’d need to create a new X11 window which has the same attributes (doable)
and the same properties (ugh) as the original window. This strikes me as
complicated and error-prone.

I’d prefer if we could keep the current approach of unmapping the window,
changing override-redirect to False and mapping it.

[Daniel Erat, 2015-12-28 21:33:02]

https://codereview.chromium.org/1456963002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1456963002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:198: bool override_redirect;
the Browser object itself isn't override-redirect; that's an implementation
detail in the X11 implementation of Views. i think that Browser should probably
either have a new Type value named TYPE_DRAGGED, or use a name like
is_being_dragged for this member.

https://codereview.chromium.org/1456963002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_aura.cc (right):

https://codereview.chromium.org/1456963002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:19: (params.type !=
views::Widget::InitParams::TYPE_WINDOW ||
did you mean '&&' here? "foo != A || foo != B" will always be true.

https://codereview.chromium.org/1456963002/diff/20001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1456963002/diff/20001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.h:60: virtual void RemapWindow() = 0;
nit: move this down to the other similar pure virtual methods below. if it's
only relevant to a particular platform or build configuration, it'd probably
make sense to #ifdef the declaration so you don't end up with a bunch of no-op
implementations on other platforms.

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:443: widget_type_ ==
Widget::InitParams::TYPE_DRAG_BROWSER))
i don't think i understand this. why is a tab that's being dragged concerned
about modality?

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:528: params.type ==
Widget::InitParams::TYPE_DRAG_BROWSER) {
same here. override-redirect windows typically don't get the focus in the first
place.

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1289:
swa.override_redirect = False;
i wouldn't have expected a method named RemapWindow() to additionally change the
window's override-redirect attribute. maybe rename it to RemapAsNormalWindow()?

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.h:162: TYPE_DRAG_BROWSER,
having views know about browser windows seems incorrect. browsers are a chrome
concept. i'd recommend just using TYPE_DRAG for these windows, or instead name
this something like TYPE_DRAG_DECORATED if the undecorated-ness of TYPE_DRAG is
a sticking point.

[stapelberg, 2016-01-18 17:22:23]

Thanks for the review. I’ve addressed all comments.

There are still some unresolved issues:

1. Dragging the last remaining tab of a window does not work.
2. There is some flickering when moving a tab from one window into another (as
opposed to moving a tab out of a window to create a new window).

I’ll look into these unless you have any other comments on the code as-is.

https://codereview.chromium.org/1456963002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/1456963002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.h:198: bool override_redirect;
On 2015/12/28 21:33:02, Daniel Erat wrote:
> the Browser object itself isn't override-redirect; that's an implementation
> detail in the X11 implementation of Views. i think that Browser should
probably
> either have a new Type value named TYPE_DRAGGED, or use a name like
> is_being_dragged for this member.

Done.

https://codereview.chromium.org/1456963002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/chrome_views_delegate_aura.cc (right):

https://codereview.chromium.org/1456963002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/chrome_views_delegate_aura.cc:19: (params.type !=
views::Widget::InitParams::TYPE_WINDOW ||
On 2015/12/28 21:33:02, Daniel Erat wrote:
> did you mean '&&' here? "foo != A || foo != B" will always be true.

Yes. In the new patchset that hunk’s no longer present, though, so this is done.

https://codereview.chromium.org/1456963002/diff/20001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1456963002/diff/20001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.h:60: virtual void RemapWindow() = 0;
On 2015/12/28 21:33:02, Daniel Erat wrote:
> nit: move this down to the other similar pure virtual methods below. if it's
> only relevant to a particular platform or build configuration, it'd probably
> make sense to #ifdef the declaration so you don't end up with a bunch of no-op
> implementations on other platforms.

Done.

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:443: widget_type_ ==
Widget::InitParams::TYPE_DRAG_BROWSER))
On 2015/12/28 21:33:02, Daniel Erat wrote:
> i don't think i understand this. why is a tab that's being dragged concerned
> about modality?

I just did a search and replace for TYPE_WINDOW and added TYPE_DRAG_WINDOW, so
there’s no deeper meaning behind this.

Since we’re going with TYPE_DRAG, this is removed in the current patchset.

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:528: params.type ==
Widget::InitParams::TYPE_DRAG_BROWSER) {
On 2015/12/28 21:33:02, Daniel Erat wrote:
> same here. override-redirect windows typically don't get the focus in the
first
> place.

Done.

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1289:
swa.override_redirect = False;
On 2015/12/28 21:33:02, Daniel Erat wrote:
> i wouldn't have expected a method named RemapWindow() to additionally change
the
> window's override-redirect attribute. maybe rename it to
RemapAsNormalWindow()?

Done.

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1456963002/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.h:162: TYPE_DRAG_BROWSER,
On 2015/12/28 21:33:02, Daniel Erat wrote:
> having views know about browser windows seems incorrect. browsers are a chrome
> concept. i'd recommend just using TYPE_DRAG for these windows, or instead name
> this something like TYPE_DRAG_DECORATED if the undecorated-ness of TYPE_DRAG
is
> a sticking point.

Done.

[Daniel Erat, 2016-01-19 20:36:22]

https://codereview.chromium.org/1456963002/diff/40001/ash/host/ash_window_tre...
File ash/host/ash_window_tree_host_unified.h (right):

https://codereview.chromium.org/1456963002/diff/40001/ash/host/ash_window_tre...
ash/host/ash_window_tree_host_unified.h:63: void RemapAsNormalWindow() override;
is this header included on all platforms? if so, this should probably have the
same desktop-linux-only ifdef, right?

https://codereview.chromium.org/1456963002/diff/40001/ash/host/ash_window_tre...
File ash/host/ash_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1456963002/diff/40001/ash/host/ash_window_tre...
ash/host/ash_window_tree_host_x11.cc:129: void
AshWindowTreeHostX11::RemapAsNormalWindow() {}
can this call NOTREACHED()? this should never be called on the host, right?

https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:363: is_being_dragged_(params.is_being_dragged),
this presumably won't compile anywhere but on desktop linux, right? rather than
adding a similar ifdef here, i think it'd be better to remove the ifdefs from
the header -- i think it's fine for other platforms to have this member, even if
they don't actually need to use it.

https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.cc:72: params.type =
views::Widget::InitParams::TYPE_DRAG;
does this cause problems on other platforms?

https://codereview.chromium.org/1456963002/diff/40001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1456963002/diff/40001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.h:174: virtual void RemapAsNormalWindow() = 0;
nit: add a comment like, "Remaps as a normal (i.e. non-override-redirect)
window."

https://codereview.chromium.org/1456963002/diff/40001/ui/aura/window_tree_hos...
File ui/aura/window_tree_host_x11.cc (right):

https://codereview.chromium.org/1456963002/diff/40001/ui/aura/window_tree_hos...
ui/aura/window_tree_host_x11.cc:554: void
WindowTreeHostX11::RemapAsNormalWindow() {
NOTREACHED() here too?

https://codereview.chromium.org/1456963002/diff/40001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/1456963002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:88: void
RemapAsNormalWindow() override;
if this needs to be public rather than protected like the other methods, please
add an "Overridden from ..." comment describing where it's originally declared

[stapelberg, 2016-01-20 13:31:09]

The current patchset behaves correctly with regards to dragging & dropping
AFAICT, i.e. the issues I mentioned in my last mail are addressed.

However, there is one thing that I could not quite track down, despite sinking
well over an hour into it. After opening a new window by dragging a tab out,
when activating that window, it shows an animation. This does not happen with
windows that were created using ctrl-n. By “activate”, I mean the _NET_WM_STATE
property being removed when it formerly was _NET_WM_STATE_HIDDEN. You can
reproduce this by using xprop:

xprop -id 0x3c00059 -f _NET_WM_STATE 32a -set _NET_WM_STATE _NET_WM_STATE_HIDDEN
xprop -id 0x3c00059 -remove _NET_WM_STATE

I used gdb to dive into the call stack a bit, and the following backtrace is
how far I got before accidentally crashing my browser and running out of time:

#0  wm::VisibilityController::UpdateLayerVisibility (this=0x39382f05c860,
window=0x100000000000000, visible=false) at
../../ui/wm/core/visibility_controller.cc:41
#1  0x000055555885cf22 in aura::Window::SetVisible (this=0x39382f05c860,
visible=true) at ../../ui/aura/window.cc:730
#2  0x000055555885cd75 in aura::Window::Show (this=0x39382f05c860) at
../../ui/aura/window.cc:228
#3  0x000055555d27bcc1 in views::DesktopWindowTreeHostX11::OnWMStateUpdated
(this=0x39382f2d03a8) at
../../ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1340
#4  0x000055555d27dc36 in views::DesktopWindowTreeHostX11::DispatchEvent
(this=0x39382f2d03a8, event=@0x7fffffffc6f0: 0x7fffffffc790) at
../../ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1997
#5  0x00005555583f9f9f in ui::PlatformEventSource::DispatchEvent
(this=0x39382e630260, platform_event=0x7fffffffc790) at
../../ui/events/platform/platform_event_source.cc:82
#6  0x000055555f2a43c7 in ui::X11EventSource::DispatchEvent
(this=0x39382e630260, xevent=0x7fffffffc790) at
../../ui/events/platform/x11/x11_event_source.cc:136
#7  0x000055555f2a42ee in ui::X11EventSource::ExtractCookieDataDispatchEvent
(this=0x39382e630260, xevent=0x7fffffffc790) at
../../ui/events/platform/x11/x11_event_source.cc:129
#8  0x000055555f2a4259 in ui::X11EventSource::DispatchXEvents
(this=0x39382e630260) at ../../ui/events/platform/x11/x11_event_source.cc:106
#9  0x000055555f2a4c95 in ui::(anonymous namespace)::XSourceDispatch
(source=0x39382e603c80, unused_func=0x0, data=0x39382e630260) at
../../ui/events/platform/x11/x11_event_source_glib.cc:39
#10 0x00007ffff70b7ce5 in g_main_dispatch (context=0x39382e46bd40) at
/build/buildd/glib2.0-2.40.2/./glib/gmain.c:3064
#11 g_main_context_dispatch (context=context@entry=0x39382e46bd40) at
/build/buildd/glib2.0-2.40.2/./glib/gmain.c:3663
#12 0x00007ffff70b8048 in g_main_context_iterate
(context=context@entry=0x39382e46bd40, block=block@entry=1,
dispatch=dispatch@entry=1, self=<optimized out>) at
/build/buildd/glib2.0-2.40.2/./glib/gmain.c:3734
#13 0x00007ffff70b80ec in g_main_context_iteration (context=0x39382e46bd40,
may_block=1) at /build/buildd/glib2.0-2.40.2/./glib/gmain.c:3795
#14 0x0000555557157d6f in base::MessagePumpGlib::Run (this=0x39382e42d700,
delegate=0x39382e5d11a0) at ../../base/message_loop/message_pump_glib.cc:309
#15 0x000055555706c02f in base::MessageLoop::RunHandler (this=0x39382e5d11a0) at
../../base/message_loop/message_loop.cc:445
#16 0x00005555570afb54 in base::RunLoop::Run (this=0x7fffffffcdd8) at
../../base/run_loop.cc:55
#17 0x0000555556942c66 in ChromeBrowserMainParts::MainMessageLoopRun
(this=0x39382e4e7c20, result_code=0x39382e4d0ab8) at
../../chrome/browser/chrome_browser_main.cc:1738
#18 0x000055555c0fb174 in content::BrowserMainLoop::RunMainMessageLoopParts
(this=0x39382e4d0aa0) at ../../content/browser/browser_main_loop.cc:889
#19 0x000055555bb4ac65 in content::BrowserMainRunnerImpl::Run
(this=0x39382e534620) at ../../content/browser/browser_main_runner.cc:214
#20 0x000055555bb4a0e8 in content::BrowserMain (parameters=...) at
../../content/browser/browser_main.cc:26
#21 0x0000555556f7d5c6 in content::RunNamedProcessTypeMain (process_type="",
main_function_params=..., delegate=0x7fffffffd9d0) at
../../content/app/content_main_runner.cc:379
#22 0x0000555556f7f07d in content::ContentMainRunnerImpl::Run
(this=0x39382e4edb60) at ../../content/app/content_main_runner.cc:804
#23 0x0000555556f7cd32 in content::ContentMain (params=...) at
../../content/app/content_main.cc:19
#24 0x00005555561326f2 in ChromeMain (argc=2, argv=0x7fffffffdb18) at
../../chrome/app/chrome_main.cc:66
#25 0x00005555561326a2 in main (argc=2, argv=0x7fffffffdb18) at
../../chrome/app/chrome_exe_main_aura.cc:17

Any ideas as to where the animation comes from, and why it is being triggered
only for windows created by dragging tabs?

https://codereview.chromium.org/1456963002/diff/40001/ash/host/ash_window_tre...
File ash/host/ash_window_tree_host_unified.h (right):

https://codereview.chromium.org/1456963002/diff/40001/ash/host/ash_window_tre...
ash/host/ash_window_tree_host_unified.h:63: void RemapAsNormalWindow() override;
On 2016/01/19 20:36:22, Daniel Erat wrote:
> is this header included on all platforms? if so, this should probably have the
> same desktop-linux-only ifdef, right?

Based on ash/BUILD.gn containing e.g. Windows-specific directives, I think
you’re right. Done.

https://codereview.chromium.org/1456963002/diff/40001/ash/host/ash_window_tre...
File ash/host/ash_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1456963002/diff/40001/ash/host/ash_window_tre...
ash/host/ash_window_tree_host_x11.cc:129: void
AshWindowTreeHostX11::RemapAsNormalWindow() {}
On 2016/01/19 20:36:22, Daniel Erat wrote:
> can this call NOTREACHED()? this should never be called on the host, right?

Done.

https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:363: is_being_dragged_(params.is_being_dragged),
On 2016/01/19 20:36:22, Daniel Erat wrote:
> this presumably won't compile anywhere but on desktop linux, right? rather
than
> adding a similar ifdef here, i think it'd be better to remove the ifdefs from
> the header -- i think it's fine for other platforms to have this member, even
if
> they don't actually need to use it.

Done.

https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.cc:72: params.type =
views::Widget::InitParams::TYPE_DRAG;
On 2016/01/19 20:36:22, Daniel Erat wrote:
> does this cause problems on other platforms?

I don’t know, I only use Linux. Do we have someone who could test this for us?

https://codereview.chromium.org/1456963002/diff/40001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/1456963002/diff/40001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.h:174: virtual void RemapAsNormalWindow() = 0;
On 2016/01/19 20:36:22, Daniel Erat wrote:
> nit: add a comment like, "Remaps as a normal (i.e. non-override-redirect)
> window."

Done.

https://codereview.chromium.org/1456963002/diff/40001/ui/aura/window_tree_hos...
File ui/aura/window_tree_host_x11.cc (right):

https://codereview.chromium.org/1456963002/diff/40001/ui/aura/window_tree_hos...
ui/aura/window_tree_host_x11.cc:554: void
WindowTreeHostX11::RemapAsNormalWindow() {
On 2016/01/19 20:36:22, Daniel Erat wrote:
> NOTREACHED() here too?

Done.

https://codereview.chromium.org/1456963002/diff/40001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/1456963002/diff/40001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:88: void
RemapAsNormalWindow() override;
On 2016/01/19 20:36:22, Daniel Erat wrote:
> if this needs to be public rather than protected like the other methods,
please
> add an "Overridden from ..." comment describing where it's originally declared

Done.

[Daniel Erat, 2016-01-23 02:18:06]

regarding the animation, i'm not sure. what does the animation look like? is it
a chrome animation or something that your window manager is doing?

https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_frame.cc:72: params.type =
views::Widget::InitParams::TYPE_DRAG;
On 2016/01/20 13:31:09, stapelberg wrote:
> On 2016/01/19 20:36:22, Daniel Erat wrote:
> > does this cause problems on other platforms?
> 
> I don’t know, I only use Linux. Do we have someone who could test this for us?

i'm in the same boat. i don't really work on chrome anymore, but when i did, i'd
usually just walk around and throw myself at the mercy of random windows and mac
developers. :-P

[stapelberg, 2016-01-25 12:26:46]

On 2016/01/23 02:18:06, Daniel Erat wrote:
> regarding the animation, i'm not sure. what does the animation look like? is
it
> a chrome animation or something that your window manager is doing?

It’s definitely a Chrome animation, since my window manager does not contain any
animations :).

I made a video: https://youtu.be/bMw-6JaJOYA

> 
>
https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_frame.cc (right):
> 
>
https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_frame.cc:72: params.type =
> views::Widget::InitParams::TYPE_DRAG;
> On 2016/01/20 13:31:09, stapelberg wrote:
> > On 2016/01/19 20:36:22, Daniel Erat wrote:
> > > does this cause problems on other platforms?
> > 
> > I don’t know, I only use Linux. Do we have someone who could test this for
us?
> 
> i'm in the same boat. i don't really work on chrome anymore, but when i did,
i'd
> usually just walk around and throw myself at the mercy of random windows and
mac
> developers. :-P

Would you happen to know whether there are any Chrome developers in the Zürich
office who would fit the profile?

[Daniel Erat, 2016-01-30 03:40:18]

On 2016/01/25 12:26:46, stapelberg wrote:
> On 2016/01/23 02:18:06, Daniel Erat wrote:
> > regarding the animation, i'm not sure. what does the animation look like? is
> it
> > a chrome animation or something that your window manager is doing?
> 
> It’s definitely a Chrome animation, since my window manager does not contain
any
> animations :).
> 
> I made a video: https://youtu.be/bMw-6JaJOYA

hmm, i don't have any ideas about that. just to be clear (since it's hard to
tell from the video), is the dragged tab getting placed in a new window, and
then that "growing" animation occurs in the new window?

> 
> > 
> >
>
https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/frame/browser_frame.cc (right):
> > 
> >
>
https://codereview.chromium.org/1456963002/diff/40001/chrome/browser/ui/views...
> > chrome/browser/ui/views/frame/browser_frame.cc:72: params.type =
> > views::Widget::InitParams::TYPE_DRAG;
> > On 2016/01/20 13:31:09, stapelberg wrote:
> > > On 2016/01/19 20:36:22, Daniel Erat wrote:
> > > > does this cause problems on other platforms?
> > > 
> > > I don’t know, I only use Linux. Do we have someone who could test this for
> us?
> > 
> > i'm in the same boat. i don't really work on chrome anymore, but when i did,
> i'd
> > usually just walk around and throw myself at the mercy of random windows and
> mac
> > developers. :-P
> 
> Would you happen to know whether there are any Chrome developers in the Zürich
> office who would fit the profile?

nope, i don't -- sorry!

[stapelberg, 2016-02-01 15:12:52]

On 2016/01/30 03:40:18, Daniel Erat wrote:
> On 2016/01/25 12:26:46, stapelberg wrote:
> > On 2016/01/23 02:18:06, Daniel Erat wrote:
> > > regarding the animation, i'm not sure. what does the animation look like?
is
> > it
> > > a chrome animation or something that your window manager is doing?
> > 
> > It’s definitely a Chrome animation, since my window manager does not contain
> any
> > animations :).
> > 
> > I made a video: https://youtu.be/bMw-6JaJOYA
> 
> hmm, i don't have any ideas about that. just to be clear (since it's hard to
> tell from the video), is the dragged tab getting placed in a new window, and
> then that "growing" animation occurs in the new window?

The “growing” animation occurs every time I focus the bottom window (I do that
by clicking the title bar, but the same happens when using the keyboard
shortcuts to focus the other window).

I created the window by dragging a tab out of the top window.

[stapelberg, 2016-10-13 08:39:48]

On 2016/02/01 15:12:52, stapelberg wrote:
> On 2016/01/30 03:40:18, Daniel Erat wrote:
> > On 2016/01/25 12:26:46, stapelberg wrote:
> > > On 2016/01/23 02:18:06, Daniel Erat wrote:
> > > > regarding the animation, i'm not sure. what does the animation look
like?
> is
> > > it
> > > > a chrome animation or something that your window manager is doing?
> > > 
> > > It’s definitely a Chrome animation, since my window manager does not
contain
> > any
> > > animations :).
> > > 
> > > I made a video: https://youtu.be/bMw-6JaJOYA
> > 
> > hmm, i don't have any ideas about that. just to be clear (since it's hard to
> > tell from the video), is the dragged tab getting placed in a new window, and
> > then that "growing" animation occurs in the new window?
> 
> The “growing” animation occurs every time I focus the bottom window (I do that
> by clicking the title bar, but the same happens when using the keyboard
> shortcuts to focus the other window).
> 
> I created the window by dragging a tab out of the top window.

This change is obsolete in favor of
https://chromium.googlesource.com/chromium/src/+/fbb8975f1693f4fb33d54f62bbef...