Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1003863005:
Fix tab drag and drop with hi-dpi. (Closed)
|
Created:
5 years, 9 months ago by stapelberg Modified:
5 years, 9 months ago CC:
chromium-reviews, tfarina, tdanderson+views_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix tab drag and drop with hi-dpi.
This change converts between DIP and pixels in a few places that were
not converted before, but are necessary for drag & drop of tabs to work
correctly.
R=sadrul@chromium.org
BUG=458804, 143619
Committed: https://crrev.com/53630bda6cd2145038f20108d13fb1cf4b979472
Cr-Commit-Position: refs/heads/master@{#321787}
Patch Set 1 #
Total comments: 8
Patch Set 2 : rename functions for clarity #
Total comments: 8
Patch Set 3 : Fix tab drag and drop with hi-dpi. #
Total comments: 5
Patch Set 4 : move conversion into window_finder_x11.cc #Patch Set 5 : noop whitespace changes #
Total comments: 4
Patch Set 6 : Fix tab drag and drop with hi-dpi. #
Messages
Total messages: 18 (3 generated)
Does this address drag-drop with other apps, or from a web-page, or does this only deal with tab-dragging? https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_screen_x11.cc:53: gfx::Point ScreenToDIPPoint(const gfx::Point& pixel_point) { Let's call this ToDIPPoint, or PixelToDIPPoint. In some places of the codebase, we use 'DIP in screen space', and so 'Screen' by itself does not necessarily mean physical pixels. PixelToDIPPoint() (or simply ToDIPPoint()) would make that clearer. https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_screen_x11.cc:60: Is there any way we can avoid dup'ing this block of code between the two files? https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_screen_x11.cc:194: const gfx::Point point = DIPToScreenPoint(requested_point); point_in_pixel https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/x11_topmost_window_finder.cc (right): https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/x11_topmost_window_finder.cc:58: screen_loc_ = DIPToScreenPoint(screen_loc); From some of the callsites, the |screen_loc| looks like it should already be in pixels?
https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_screen_x11.cc:53: gfx::Point ScreenToDIPPoint(const gfx::Point& pixel_point) { On 2015/03/17 15:56:06, sadrul wrote: > Let's call this ToDIPPoint, or PixelToDIPPoint. > > In some places of the codebase, we use 'DIP in screen space', and so 'Screen' by > itself does not necessarily mean physical pixels. PixelToDIPPoint() (or simply > ToDIPPoint()) would make that clearer. Done. https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_screen_x11.cc:60: On 2015/03/17 15:56:06, sadrul wrote: > Is there any way we can avoid dup'ing this block of code between the two files? I wouldn’t know how, if not by introducing yet another file that contains these functions. At that point we’d have ui/compositor/dip_util and ui/gfx/win/dpi and our new file doing the same thing. I think it’d be worthwhile to clean this up across chromium, but I feel like that’s out of scope for the bugfix I’m trying to get in :). I tried using ash::Shell::GetPrimaryRootWindow so that I can use ui/compositor/dip_util, but that didn’t work, chromium crashes with the following backtrace when I try that: #0 0x00007ffff177a8c7 in raise () at /lib64/libc.so.6 #1 0x00007ffff177c52a in abort () at /lib64/libc.so.6 #2 0x0000555556553956 in () #3 0x00005555565672e8 in logging::LogMessage::~LogMessage() () #4 0x000055555840a9c5 in ash::Shell::GetPrimaryRootWindow() () #5 0x0000555558c0b0f6 in views::DesktopScreenX11::GetCursorScreenPoint() () #6 0x0000555558558ca6 in content::RenderWidgetHostViewAura::UpdateCursorIfOverSelf() () #7 0x000055555854824c in content::RenderViewHostImpl::SetIsLoading(bool) () #8 0x00005555584d3add in content::RenderFrameHostManager::SetIsLoading(bool) () #9 0x00005555585dc0f1 in content::WebContentsImpl::SetIsLoading(content::RenderViewHost*, bool, bool, content::LoadNotificationDetails*) () #10 0x00005555585df3c0 in non-virtual thunk to content::WebContentsImpl::DidStartLoading(content::RenderFrameHost*, bool) () #11 0x00005555584cf730 in content::RenderFrameHostImpl::Navigate(content::CommonNavigationParams const&, content::StartNavigationParams const&, content::CommitNavigationParams const&, content::HistoryNavigationParams const&) () #12 0x00005555586ae007 in content::NavigatorImpl::NavigateToEntry(content::FrameTreeNode*, content::NavigationEntryImpl const&, content::NavigationController::ReloadType) () #13 0x00005555584c0c53 in content::NavigationControllerImpl::NavigateToPendingEntry(content::NavigationController::ReloadType) () #14 0x00005555584c0eea in content::NavigationControllerImpl::LoadEntry(content::NavigationEntryImpl*) () #15 0x00005555584c1937 in content::NavigationControllerImpl::LoadURLWithParams(content::NavigationController::LoadURLParams const&) () #16 0x00005555580dc819 in (anonymous namespace)::LoadURLInContents(content::WebContents*, GURL const&, chrome::NavigateParams*) () #17 0x00005555580dc0a1 in chrome::Navigate(chrome::NavigateParams*) () #18 0x00005555581e2fea in StartupBrowserCreatorImpl::OpenTabsInBrowser(Browser*, bool, std::vector<StartupTab, std::allocator<StartupTab> > const&, chrome::HostDesktopType) () #19 0x00005555581e210f in StartupBrowserCreatorImpl::ProcessSpecifiedURLs(std::vector<GURL, std::allocator<GURL> > const&, chrome::HostDesktopType) () #20 0x00005555581e1dd6 in StartupBrowserCreatorImpl::ProcessStartupURLs(std::vector<GURL, std::allocator<GURL> > const&, chrome::HostDesktopType) () #21 0x00005555581e139d in StartupBrowserCreatorImpl::ProcessLaunchURLs(bool, std::vector<GURL, std::allocator<GURL> > const&, chrome::HostDesktopType) () #22 0x00005555581e0960 in StartupBrowserCreatorImpl::Launch(Profile*, std::vector<GURL, std::allocator<GURL> > const&, bool, chrome::HostDesktopType) () #23 0x00005555581de7ac in StartupBrowserCreator::LaunchBrowser(base::CommandLine const&, Profile*, base::FilePath const&, chrome::startup::IsProcessStartup, chrome::startup::IsFirstRun) () #24 0x00005555581de16d in StartupBrowserCreator::ProcessCmdLineImpl(base::CommandLine const&, base::FilePath const&, bool, Profile*, std::vector<Profile*, std::allocator<Profile*> > const&, StartupBrowserCreator*) () #25 0x00005555581dcb18 in StartupBrowserCreator::Start(base::CommandLine const&, base::FilePath const&, Profile*, std::vector<Profile*, std::allocator<Profile*> > const&) () #26 0x000055555626bc7b in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() () #27 0x000055555626a992 in ChromeBrowserMainParts::PreMainMessageLoopRun() () #28 0x000055555866f6a9 in content::BrowserMainLoop::PreMainMessageLoopRun() () #29 0x0000555558754b47 in content::StartupTaskRunner::RunAllTasksNow() () #30 0x000055555866de1e in content::BrowserMainLoop::CreateStartupTasks() () #31 0x000055555848b244 in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) () #32 0x000055555848af8d in content::BrowserMain(content::MainFunctionParams const&) () #33 0x00005555564fb2b5 in content::ContentMainRunnerImpl::Run() () #34 0x00005555564f9f10 in content::ContentMain(content::ContentMainParams const&) () #35 0x0000555555fe7c68 in ChromeMain () #36 0x00007ffff1765fe0 in __libc_start_main () at /lib64/libc.so.6 #37 0x0000555555fe7b1f in _start () So, in conclusion, unless you see an easy way, I’d ask you to push this refactoring off for later. https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_screen_x11.cc:194: const gfx::Point point = DIPToScreenPoint(requested_point); On 2015/03/17 15:56:06, sadrul wrote: > point_in_pixel Done. https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/x11_topmost_window_finder.cc (right): https://codereview.chromium.org/1003863005/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/x11_topmost_window_finder.cc:58: screen_loc_ = DIPToScreenPoint(screen_loc); On 2015/03/17 15:56:06, sadrul wrote: > From some of the callsites, the |screen_loc| looks like it should already be in > pixels? You’re right. Reverted that part of the change.
https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:156: return finder.FindLocalProcessWindowAt(point, std::set<aura::Window*>()); Do we need to changes in X11TopmostWindowFinder if we convert to pixel-space in here? (if you make this change, please update the comment on X11TopmostWindowFinder to change |screen_loc| to |screen_loc_in_pixels|) https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_topmost_window_finder.cc (right): https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_topmost_window_finder.cc:34: screen_loc_ = DIPToPixelPoint(screen_loc); Since FindWindowAt() takes screen_loc in physical-pixel space, I think it would be better to have FindLocalProcessWindowAt() also take screen_loc in physical-pixels. Please make sure to append |in_pixels| to the variable names to clarify this.
https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_topmost_window_finder.cc (right): https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_topmost_window_finder.cc:103: screen_position_client->ConvertPointFromScreen(window, &window_loc); Oh, also, does this do the right thing? (I seem to remember the ScreenPositionClient actually working in DIP)
https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:156: return finder.FindLocalProcessWindowAt(point, std::set<aura::Window*>()); On 2015/03/19 23:46:58, sadrul wrote: > Do we need to changes in X11TopmostWindowFinder if we convert to pixel-space in > here? (if you make this change, please update the comment on > X11TopmostWindowFinder to change |screen_loc| to |screen_loc_in_pixels|) Done. https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_topmost_window_finder.cc (right): https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_topmost_window_finder.cc:34: screen_loc_ = DIPToPixelPoint(screen_loc); On 2015/03/19 23:46:58, sadrul wrote: > Since FindWindowAt() takes screen_loc in physical-pixel space, I think it would > be better to have FindLocalProcessWindowAt() also take screen_loc in > physical-pixels. Please make sure to append |in_pixels| to the variable names to > clarify this. Done. https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_topmost_window_finder.cc:103: screen_position_client->ConvertPointFromScreen(window, &window_loc); On 2015/03/19 23:47:40, sadrul wrote: > Oh, also, does this do the right thing? (I seem to remember the > ScreenPositionClient actually working in DIP) I’m not sure — the code path never gets executed for me. How can I make chromium use a non-rectangular window shape at all?
https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_topmost_window_finder.cc (right): https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_topmost_window_finder.cc:103: screen_position_client->ConvertPointFromScreen(window, &window_loc); On 2015/03/20 08:34:40, stapelberg wrote: > On 2015/03/19 23:47:40, sadrul wrote: > > Oh, also, does this do the right thing? (I seem to remember the > > ScreenPositionClient actually working in DIP) > > I’m not sure — the code path never gets executed for me. How can I make chromium > use a non-rectangular window shape at all? Ah, you need some chrome app that sets custom windows (I think hangouts, keep apps do). It would be OK if you want to address this separately from this patch. https://codereview.chromium.org/1003863005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1003863005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1814: #endif This should actually happen in window_finder_x11.cc, right? That would make most sense, since on Windows, we use DIP in the call to GetLocalProcessWindowAtPoint(), and so we should also use DIP in the call to GetLocalProcessWindowAtPoint() on Linux, but convert to pixels before calling into X11TopmostWindowFinder. https://codereview.chromium.org/1003863005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.h (right): https://codereview.chromium.org/1003863005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.h:447: gfx::NativeWindow GetLocalProcessWindow(gfx::Point screen_point, Why this change?
https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_topmost_window_finder.cc (right): https://codereview.chromium.org/1003863005/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_topmost_window_finder.cc:103: screen_position_client->ConvertPointFromScreen(window, &window_loc); On 2015/03/20 17:40:23, sadrul wrote: > On 2015/03/20 08:34:40, stapelberg wrote: > > On 2015/03/19 23:47:40, sadrul wrote: > > > Oh, also, does this do the right thing? (I seem to remember the > > > ScreenPositionClient actually working in DIP) > > > > I’m not sure — the code path never gets executed for me. How can I make > chromium > > use a non-rectangular window shape at all? > > Ah, you need some chrome app that sets custom windows (I think hangouts, keep > apps do). > > It would be OK if you want to address this separately from this patch. I see. I’d like to postpone this until later, and focus on getting the 99% use-case in first :). https://codereview.chromium.org/1003863005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1003863005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1814: #endif On 2015/03/20 17:40:24, sadrul wrote: > This should actually happen in window_finder_x11.cc, right? That would make most > sense, since on Windows, we use DIP in the call to > GetLocalProcessWindowAtPoint(), and so we should also use DIP in the call to > GetLocalProcessWindowAtPoint() on Linux, but convert to pixels before calling > into X11TopmostWindowFinder. Done. I really hope you won’t suggest _another_ location to try now :). https://codereview.chromium.org/1003863005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.h (right): https://codereview.chromium.org/1003863005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.h:447: gfx::NativeWindow GetLocalProcessWindow(gfx::Point screen_point, On 2015/03/20 17:40:24, sadrul wrote: > Why this change? It saved a conditional branch by overwriting the screen_point variable. But this reverted now anyway :).
LGTM! (you will need another owner for the change in chrome/) https://codereview.chromium.org/1003863005/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/1003863005/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:1814: #endif On 2015/03/20 21:48:06, stapelberg wrote: > On 2015/03/20 17:40:24, sadrul wrote: > > This should actually happen in window_finder_x11.cc, right? That would make > most > > sense, since on Windows, we use DIP in the call to > > GetLocalProcessWindowAtPoint(), and so we should also use DIP in the call to > > GetLocalProcessWindowAtPoint() on Linux, but convert to pixels before calling > > into X11TopmostWindowFinder. > > Done. I really hope you won’t suggest _another_ location to try now :). Heh no. The new patchset looks good. We need to make sure that the API we use is mostly consistent across platforms, and so it's important to make sure we are using the same coordinate-space on both Windows and Linux for GetLocalProcessWindowAtPoint() (which, in this case, happens to be DIP). https://codereview.chromium.org/1003863005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/window_finder_x11.cc (right): https://codereview.chromium.org/1003863005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/window_finder_x11.cc:25: These two functions should be in anonymous namespace (at the top of this file). https://codereview.chromium.org/1003863005/diff/80001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_topmost_window_finder.h (right): https://codereview.chromium.org/1003863005/diff/80001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_topmost_window_finder.h:44: gfx::Point screen_loc_; Add _in_pixels_ here too
https://codereview.chromium.org/1003863005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/window_finder_x11.cc (right): https://codereview.chromium.org/1003863005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/window_finder_x11.cc:25: On 2015/03/20 22:02:03, sadrul wrote: > These two functions should be in anonymous namespace (at the top of this file). Done. https://codereview.chromium.org/1003863005/diff/80001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_topmost_window_finder.h (right): https://codereview.chromium.org/1003863005/diff/80001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_topmost_window_finder.h:44: gfx::Point screen_loc_; On 2015/03/20 22:02:03, sadrul wrote: > Add _in_pixels_ here too Done.
stapelberg@google.com changed reviewers: + sky@chromium.org
sky, could you please take a look at this for OWNERS approval? Thank you!
LGTM
The CQ bit was checked by stapelberg@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1003863005/#ps100001 (title: "Fix tab drag and drop with hi-dpi.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1003863005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/53630bda6cd2145038f20108d13fb1cf4b979472 Cr-Commit-Position: refs/heads/master@{#321787} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
