Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
225403022:
Fixed positioning of tooltips in multi monitor setups. (Closed)
|
Created:
6 years, 8 months ago by Mateusz Szymański Modified:
6 years, 7 months ago CC:
chromium-reviews, tfarina, ben+corewm_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFixed positioning of tooltips in multi monitor setups.
If window is positioned on right monitor in multimonitor setup
tooltips for items to the left of Chromium window appear on left screen.
This is because screen on which to display the tooltip is calculated from
DIP coordinates.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272218
Patch Set 1 #
Total comments: 3
Patch Set 2 : Unhardcoded cursor size hence simplifying DIP to screen conversions #
Total comments: 4
Patch Set 3 : what could possibly go wrong #Patch Set 4 : Fixed undefined behavior while calculating mask in certain edge cases. #Patch Set 5 : Fixed tooltip positioning near the screen edges." #Patch Set 6 : Squashed patchset. #
Total comments: 15
Patch Set 7 : Fixed tooltip positioning in HiDPI systems and with custom cursor arrows. #
Total comments: 7
Patch Set 8 : More review fixes. #
Messages
Total messages: 36 (0 generated)
Looks reasonable, but Windows change --> sky@ https://codereview.chromium.org/225403022/diff/1/ui/views/corewm/tooltip_win.cc File ui/views/corewm/tooltip_win.cc (right): https://codereview.chromium.org/225403022/diff/1/ui/views/corewm/tooltip_win.... ui/views/corewm/tooltip_win.cc:93: tooltip_bounds.AdjustToFit(display.work_area()); Does |tooltip_bounds| need to be in screen coordinate here?
https://codereview.chromium.org/225403022/diff/1/ui/views/corewm/tooltip_win.cc File ui/views/corewm/tooltip_win.cc (right): https://codereview.chromium.org/225403022/diff/1/ui/views/corewm/tooltip_win.... ui/views/corewm/tooltip_win.cc:99: tooltip_bounds = gfx::win::DIPToScreenRect(tooltip_bounds); Notice the conversion here too. Seems like it should be possible to only do conversion once.
+ananta who did some tweaks here too. -sadrul
https://codereview.chromium.org/225403022/diff/1/ui/views/corewm/tooltip_win.cc File ui/views/corewm/tooltip_win.cc (right): https://codereview.chromium.org/225403022/diff/1/ui/views/corewm/tooltip_win.... ui/views/corewm/tooltip_win.cc:93: tooltip_bounds.AdjustToFit(display.work_area()); On 2014/04/10 16:18:36, sadrul wrote: > Does |tooltip_bounds| need to be in screen coordinate here? It's in DIP coordinates, notice the conversion in line 99.
Is there really no API we can use to get the height? +cpu https://codereview.chromium.org/225403022/diff/20001/ui/views/corewm/cursor_h... File ui/views/corewm/cursor_height_provider_win.h (right): https://codereview.chromium.org/225403022/diff/20001/ui/views/corewm/cursor_h... ui/views/corewm/cursor_height_provider_win.h:16: /** This isn't Java;) Use // for comments ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml ). https://codereview.chromium.org/225403022/diff/20001/ui/views/corewm/cursor_h... ui/views/corewm/cursor_height_provider_win.h:29: int GetCursorHeight(); GetCurrentCursorHeight() https://codereview.chromium.org/225403022/diff/20001/ui/views/corewm/cursor_h... ui/views/corewm/cursor_height_provider_win.h:34: HeightStorage cached_heights_; Shouldn't this be shared among instances? In fact is there a reason GetCurrentCursorHeight() shouldn't just be a standalone function?
I am as surprised as sky there's no pre-existing API for that, at least none that I'm aware of. https://codereview.chromium.org/225403022/diff/20001/ui/views/corewm/cursor_h... File ui/views/corewm/cursor_height_provider_win.h (right): https://codereview.chromium.org/225403022/diff/20001/ui/views/corewm/cursor_h... ui/views/corewm/cursor_height_provider_win.h:34: HeightStorage cached_heights_; On 2014/04/30 17:22:14, sky wrote: > Shouldn't this be shared among instances? In fact is there a reason > GetCurrentCursorHeight() shouldn't just be a standalone function? Come to think about it it should have been a function operating on global variable. Will adjust that.
sorry I am in training today. If ananta can answer the question or else plese wait 8 more hours.
On 2014/05/06 19:54:43, cpu wrote: > sorry I am in training today. If ananta can answer the question or else plese > wait 8 more hours. GetSystemMetrics(SM_CYCURSOR) should give you the height of the cursor in pixels. http://msdn.microsoft.com/en-us/library/windows/desktop/ms724385(v=vs.85).aspx
On 2014/05/06 23:51:24, ananta wrote: > On 2014/05/06 19:54:43, cpu wrote: > > sorry I am in training today. If ananta can answer the question or else plese > > wait 8 more hours. > > GetSystemMetrics(SM_CYCURSOR) should give you the height of the cursor in > pixels. > http://msdn.microsoft.com/en-us/library/windows/desktop/ms724385(v=vs.85).aspx Scratch that. Will look to see if there is a way to get the visible height of the cursor.
Ping? I'd love to know if this looks to you like a solution that's acceptable after some polishing or a madman's idea that'll never make it to the mainline, so that I can decide whether to fix some remaining quirks or not ;)
On 2014/05/12 09:52:44, Mateusz Szymański wrote: > Ping? > I'd love to know if this looks to you like a solution that's acceptable after > some polishing or a madman's idea that'll never make it to the mainline, so that > I can decide whether to fix some remaining quirks or not ;) The approach looks fine to me.
lgtm
The CQ bit was checked by mateuszs@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mateuszs@opera.com/225403022/100001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
The CQ bit was checked by mateuszs@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mateuszs@opera.com/225403022/120001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
Can you please LGTM again if it looks good? CommitQueue disregarded previous approval.
https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... File ui/views/corewm/cursor_height_provider_win.cc (right): https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:15: typedef scoped_ptr<uint32> PixelData; uint32 is deprecated. Use uint32_t (same comment all through this file). https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:18: const uint32 bits_per_uint32 = 32; Why not use sizeof(uint32_t) * 8 when you need this? https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:20: const uint32 transparent_mask = 0xffffffff; Use k for constants, eg kTransparentMask. https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:24: static HeightStorage* cached_heights = nullptr; We're not using c++ 11 yet. https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:42: if (header == nullptr) Don't null check news. If new fails we'll crash. https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:47: if (data == nullptr) Same comment here about checking for null. https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:50: int result = GetDIBits(hdc, Shouldn't you check the return value? Perhaps resetting data if not success? https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:62: bool IsRowTransparent(const PixelData& data, const uint32 row_size, nit: when you wrap, one param per line. https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:104: if (data == nullptr) no null check https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:128: if (cached_heights == nullptr) { nit: no {} https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... File ui/views/corewm/cursor_height_provider_win.h (right): https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.h:16: int GetCursorHeight(); This returns information based on the current cursor, right? Also, since it's the visible height and not necessarily the real height, how about naming it GetCurrentCursorVisibleHeight?
https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... File ui/views/corewm/cursor_height_provider_win.cc (right): https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:18: const uint32 bits_per_uint32 = 32; On 2014/05/16 17:13:22, sky wrote: > Why not use sizeof(uint32_t) * 8 when you need this? Because uint32 is obviously 32 bits long and if this ever changes ban the person who makes the change? :P I can adjust it, no problem. https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:50: int result = GetDIBits(hdc, On 2014/05/16 17:13:22, sky wrote: > Shouldn't you check the return value? Perhaps resetting data if not success? Nicely spotted, I was sure I did. https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... File ui/views/corewm/cursor_height_provider_win.h (right): https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.h:16: int GetCursorHeight(); On 2014/05/16 17:13:22, sky wrote: > This returns information based on the current cursor, right? Also, since it's > the visible height and not necessarily the real height, how about naming it > GetCurrentCursorVisibleHeight? OK, sounds fine to me.
https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... File ui/views/corewm/cursor_height_provider_win.cc (right): https://codereview.chromium.org/225403022/diff/120001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:104: if (data == nullptr) On 2014/05/16 17:13:22, sky wrote: > no null check NULL here is return value indicating internal error.
https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... File ui/views/corewm/cursor_height_provider_win.cc (right): https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:28: static HeightStorage* g_cached_heights = NULL; g_cached_heights -> cached_heights (g_ is for globals, cached_heights is file local). https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:96: if (bits_to_shift != kBitsPeruint32) { nit: no {} https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:112: i--; Might you set i to -1 here? https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... File ui/views/corewm/cursor_height_provider_win.h (right): https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.h:15: nit: remove empty line.
https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... File ui/views/corewm/cursor_height_provider_win.cc (right): https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:112: i--; On 2014/05/19 16:12:46, sky wrote: > Might you set i to -1 here? Yes, I believe I can and I believe that would be correct if the cursor is SM_CYCURSOR high - in that case I have to push the tooltip one pixel lower.
https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... File ui/views/corewm/cursor_height_provider_win.cc (right): https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:112: i--; On 2014/05/20 09:49:52, Mateusz Szymański wrote: > On 2014/05/19 16:12:46, sky wrote: > > Might you set i to -1 here? > > Yes, I believe I can and I believe that would be correct if the cursor is > SM_CYCURSOR high - in that case I have to push the tooltip one pixel lower. Might that mean you're going to return a cursor height of -1? That doesn't sound right.
https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... File ui/views/corewm/cursor_height_provider_win.cc (right): https://codereview.chromium.org/225403022/diff/140001/ui/views/corewm/cursor_... ui/views/corewm/cursor_height_provider_win.cc:112: i--; On 2014/05/20 17:19:23, sky wrote: > Might that mean you're going to return a cursor height of -1? That doesn't sound > right. Note that the value returned is return bitmap_info.bmiHeader.biHeight - i - icon.yHotspot; in line 116, not i.
Good point. LGTM
The CQ bit was checked by mateuszs@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mateuszs@opera.com/225403022/160001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 272218 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
