Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
180983002:
Avoids releasing capture prematurily when dragging tabs from one to another browser window (Closed)
|
Created:
6 years, 10 months ago by varkha Modified:
6 years, 9 months ago CC:
chromium-reviews, tfarina, dcheng, sadrul, sky Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAvoids releasing capture prematurily when dragging tabs from one to another browser window
BUG=338297
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255826
Patch Set 1 #Patch Set 2 : Avoids releasing capture prematurily when dragging tabs from one browser window to another #
Total comments: 8
Patch Set 3 : try-bot fails - selective revert #Patch Set 4 : Uses Aura mouse capture and release code path on Linux #
Messages
Total messages: 25 (0 generated)
erg@, could you please take a look? I am not 100% this is an optimal solution but it seems to work well in all cases I could think of. Thanks! https://codereview.chromium.org/180983002/diff/10001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/180983002/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:701: browser_widget->Hide(); Not calling Hide on Linux seems to be lesser evil (not even sure if the concern was valid on Linux). https://codereview.chromium.org/180983002/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1217: tab_strip_to_attach_to_after_exit_->GetWidget()->Activate(); Activating before calling Detach (which may close the browser frame) helps to not lose capture prematurely. https://codereview.chromium.org/180983002/diff/10001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/180983002/diff/10001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_desktop_handler.cc:98: OnActiveWindowChanged(window); Without this when a tab that is dragged from one browser to another has focus in a text field that focus gets lost after the drag. Calling it synchronously helps and focus is maintained (avoiding symptoms from http://crbug.com/275329. Is there a better way?
https://codereview.chromium.org/180983002/diff/10001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/180983002/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:697: #if !defined(OS_LINUX) Isn't this code used on chromeos, too? https://codereview.chromium.org/180983002/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1217: tab_strip_to_attach_to_after_exit_->GetWidget()->Activate(); On 2014/02/26 20:30:18, varkha wrote: > Activating before calling Detach (which may close the browser frame) helps to > not lose capture prematurely. What's the effect on Windows here? https://codereview.chromium.org/180983002/diff/10001/ui/views/widget/desktop_... File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right): https://codereview.chromium.org/180983002/diff/10001/ui/views/widget/desktop_... ui/views/widget/desktop_aura/x11_desktop_handler.cc:98: OnActiveWindowChanged(window); On 2014/02/26 20:30:18, varkha wrote: > Without this when a tab that is dragged from one browser to another has focus in > a text field that focus gets lost after the drag. Calling it synchronously helps > and focus is maintained (avoiding symptoms from http://crbug.com/275329. Is > there a better way? Not that I know of. sadrul@ might be a better person to ask though, since he has more in depth knowledge of X11 lore.
https://codereview.chromium.org/180983002/diff/10001/chrome/browser/ui/views/... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/180983002/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:697: #if !defined(OS_LINUX) On 2014/02/26 20:33:17, Elliot Glaysher wrote: > Isn't this code used on chromeos, too? It is, but I did not see any visual impact from skipping the call to Hide() on Chrome OS. I suspect that the reason for having it here was not relevant on Chrome OS. https://codereview.chromium.org/180983002/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1217: tab_strip_to_attach_to_after_exit_->GetWidget()->Activate(); On 2014/02/26 20:33:17, Elliot Glaysher wrote: > On 2014/02/26 20:30:18, varkha wrote: > > Activating before calling Detach (which may close the browser frame) helps to > > not lose capture prematurely. > > What's the effect on Windows here? I didn't have a chance to test this on Windows yet (TBD). I hope that this is benign since I am moving it earlier.
erg@, sky@, I have been looking at this some more. PTAL. I think the "ash" path in TabDragController should really apply to Linux Aura build. This also avoids changing Windows or any other platform.
lgtm
Have you verified this on windows? On Fri, Mar 7, 2014 at 10:52 AM, <varkha@chromium.org> wrote: > erg@, sky@, I have been looking at this some more. PTAL. I think the "ash" > path > in TabDragController should really apply to Linux Aura build. This also > avoids > changing Windows or any other platform. > > https://codereview.chromium.org/180983002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/07 21:51:29, sky wrote: > Have you verified this on windows? The change should not affect Windows, should it (the changed code should all be under if defined(LINUX))?
On 2014/03/07 21:55:16, varkha wrote: > On 2014/03/07 21:51:29, sky wrote: > > Have you verified this on windows? > > The change should not affect Windows, should it (the changed code should all be > under if defined(LINUX))? ... and yes, I did run it on Windows - no change as expected.
LGTM
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/180983002/170001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/180983002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/180983002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/180983002/170001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by varkha@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/180983002/170001
Message was sent while issue was closed.
Change committed as 255826 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
