Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1070433002:
device scale factor detection: use gtk-xft-dpi consistently (Closed)
|
Created:
5 years, 8 months ago by stapelberg Modified:
5 years, 8 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. |
Descriptiondevice scale factor detection: use gtk-xft-dpi consistently
Before this CL, we used either gtk-xft-dpi (UI fonts) or the physical DPI of
a more or less random display. This CL consistently uses gtk-xft-dpi which
is the better alternative: users can directly influence it (by changing the
Xft.dpi X resource, or via their gtkrc).
See also https://github.com/derat/font-config-info for displaying your
current system settings.
BUG=473089, 143619
Committed: https://crrev.com/06cda64742b45e1eb10bbbb66bca4b4da2efb3c9
Cr-Commit-Position: refs/heads/master@{#324782}
Patch Set 1 #
Total comments: 2
Patch Set 2 : device scale factor detection: always use X11 root window’s dpi #
Total comments: 5
Patch Set 3 : device scale factor detection: always use X11 root window’s dpi #
Total comments: 4
Patch Set 4 : device scale factor detection: use gtk-xft-dpi consistently #Patch Set 5 : device scale factor detection: use gtk-xft-dpi consistently #Patch Set 6 : device scale factor detection: use gtk-xft-dpi consistently #
Messages
Total messages: 38 (13 generated)
stapelberg@google.com changed reviewers: + sadrul@chromium.org
sadrul@chromium.org changed reviewers: + oshima@chromium.org
+oshima@ for review.
https://codereview.chromium.org/1070433002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/1070433002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_screen_x11.cc:38: // TODO(oshima): Consider using gtk-xft-dpi instead. We should use gtk-xft-dpi instead, shouldn't we? This is what we use for font size (Gtk2UI::UpdateDefaultFont), so using this allow us to scale when a user changes the font scale (using gnome-tweak-tool for example).
On 2015/04/08 17:33:58, oshima wrote: > https://codereview.chromium.org/1070433002/diff/1/ui/views/widget/desktop_aur... > File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): > > https://codereview.chromium.org/1070433002/diff/1/ui/views/widget/desktop_aur... > ui/views/widget/desktop_aura/desktop_screen_x11.cc:38: // TODO(oshima): Consider > using gtk-xft-dpi instead. > We should use gtk-xft-dpi instead, shouldn't we? This is what we use for font > size (Gtk2UI::UpdateDefaultFont), so using this allow us to scale when a user > changes the font scale (using gnome-tweak-tool for example). I think it makes sense to make the two places in the code consistent and don’t mind too much which method we use for figuring out the DPI. Are you okay with adding a gtk dependency to //ui/views or should I add a method to Gtk2UI which returns the DPI?
On 2015/04/08 17:59:56, stapelberg wrote: > On 2015/04/08 17:33:58, oshima wrote: > > > https://codereview.chromium.org/1070433002/diff/1/ui/views/widget/desktop_aur... > > File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): > > > > > https://codereview.chromium.org/1070433002/diff/1/ui/views/widget/desktop_aur... > > ui/views/widget/desktop_aura/desktop_screen_x11.cc:38: // TODO(oshima): > Consider > > using gtk-xft-dpi instead. > > We should use gtk-xft-dpi instead, shouldn't we? This is what we use for font > > size (Gtk2UI::UpdateDefaultFont), so using this allow us to scale when a user > > changes the font scale (using gnome-tweak-tool for example). > > I think it makes sense to make the two places in the code consistent and don’t > mind too much which method we use for figuring out the DPI. > > Are you okay with adding a gtk dependency to //ui/views or should I add a method > to Gtk2UI which returns the DPI? You should be able to use it via views::LinuxUI without adding dependency. LinuxUI::SetDeviceScaleFactor currently updates the font size using display's scale factor, so you need to flip it and let the font scale decide display's scale factor. (You should be able to remove it, I think)
https://codereview.chromium.org/1070433002/diff/1/ui/views/widget/desktop_aur... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/1070433002/diff/1/ui/views/widget/desktop_aur... ui/views/widget/desktop_aura/desktop_screen_x11.cc:38: // TODO(oshima): Consider using gtk-xft-dpi instead. On 2015/04/08 17:33:58, oshima wrote: > We should use gtk-xft-dpi instead, shouldn't we? This is what we use for font > size (Gtk2UI::UpdateDefaultFont), so using this allow us to scale when a user > changes the font scale (using gnome-tweak-tool for example). Done.
lgtm with nits https://codereview.chromium.org/1070433002/diff/20001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right): https://codereview.chromium.org/1070433002/diff/20001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1430: return ui::GetScaleForScaleFactor(ui::GetSupportedScaleFactor(scale)); you may want to add more scales for linux (133 150 etc). https://codereview.chromium.org/1070433002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/1070433002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:288: device_scale_factor = fallback_displays[0].device_scale_factor(); nit: you can get device scale factor from LinuxUI directly. no need to create display list. float device_scale_factor = LinixUI::instance()->GetDeviceScaleFactor();
https://codereview.chromium.org/1070433002/diff/20001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right): https://codereview.chromium.org/1070433002/diff/20001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1430: return ui::GetScaleForScaleFactor(ui::GetSupportedScaleFactor(scale)); On 2015/04/08 23:57:41, oshima wrote: > you may want to add more scales for linux (133 150 etc). Yeah, I’ll investigate that in a separate CL so that we can revert them separately if necessary. https://codereview.chromium.org/1070433002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/1070433002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:288: device_scale_factor = fallback_displays[0].device_scale_factor(); On 2015/04/08 23:57:41, oshima wrote: > nit: you can get device scale factor from LinuxUI directly. no need to create > display list. > > float device_scale_factor = LinixUI::instance()->GetDeviceScaleFactor(); Done.
stapelberg@google.com changed reviewers: + erg@google.com
erg, can you please take a look for OWNERS approval? Thank you.
erg@chromium.org changed reviewers: + erg@chromium.org
https://codereview.chromium.org/1070433002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/1070433002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:284: { Is there a reason this is in a block instead of just float device_scale_factor = views::LinuxUI::instance()->GetDeviceScaleFactor()? (It looks like you started out with a temp vector in here, but now that that's gone, there's no reason to have this scope.)
https://codereview.chromium.org/1070433002/diff/40001/ui/views/linux_ui/linux... File ui/views/linux_ui/linux_ui.h (right): https://codereview.chromium.org/1070433002/diff/40001/ui/views/linux_ui/linux... ui/views/linux_ui/linux_ui.h:156: virtual void UpdateDeviceScaleFactor(float device_scale_factor) = 0; Given that LinuxUI is now responsible for getting the DSF (which is used to set the DSF on the display), do we still need UpdateDeviceScaleFactor()?
https://codereview.chromium.org/1070433002/diff/20001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right): https://codereview.chromium.org/1070433002/diff/20001/chrome/browser/ui/libgt... chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1430: return ui::GetScaleForScaleFactor(ui::GetSupportedScaleFactor(scale)); On 2015/04/09 06:49:34, stapelberg wrote: > On 2015/04/08 23:57:41, oshima wrote: > > you may want to add more scales for linux (133 150 etc). > > Yeah, I’ll investigate that in a separate CL so that we can revert them > separately if necessary. I think my comment was a bit confusing. Technically, you can just pass scale, although we don't want to allow arbitrary scale factor. On Linux, we can pick something like 1.25, 1.5 and 2.0 and use closest scale factor. (ChromeOS currently support 1.0, 1.25 and 2.0)
On 2015/04/09 17:22:23, oshima wrote: > https://codereview.chromium.org/1070433002/diff/20001/chrome/browser/ui/libgt... > File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right): > > https://codereview.chromium.org/1070433002/diff/20001/chrome/browser/ui/libgt... > chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1430: return > ui::GetScaleForScaleFactor(ui::GetSupportedScaleFactor(scale)); > On 2015/04/09 06:49:34, stapelberg wrote: > > On 2015/04/08 23:57:41, oshima wrote: > > > you may want to add more scales for linux (133 150 etc). > > > > Yeah, I’ll investigate that in a separate CL so that we can revert them > > separately if necessary. > > I think my comment was a bit confusing. Technically, you can just pass scale, > although we don't want to allow arbitrary scale factor. On Linux, we can pick > something like 1.25, 1.5 and 2.0 and use closest scale factor. (ChromeOS > currently support 1.0, 1.25 and 2.0) I’m not sure I understand what you’re saying. Currently, we are choosing either 1.0 or 2.0 on Linux because these are the only supported scale factors. If you want me to add more scale factors then I’d like to prefer to do that in a separate CL as explained in my previous comment.
PTAL. https://codereview.chromium.org/1070433002/diff/40001/ui/views/linux_ui/linux... File ui/views/linux_ui/linux_ui.h (right): https://codereview.chromium.org/1070433002/diff/40001/ui/views/linux_ui/linux... ui/views/linux_ui/linux_ui.h:156: virtual void UpdateDeviceScaleFactor(float device_scale_factor) = 0; On 2015/04/09 16:50:01, sadrul wrote: > Given that LinuxUI is now responsible for getting the DSF (which is used to set > the DSF on the display), do we still need UpdateDeviceScaleFactor()? Yes. If I try to call GetDeviceScaleFactor() directly in UpdateDefaultFont(), I get a segfault because the supported scale factors have not yet been initialized (we’ve had the same thing in another CL a while ago): #0 0x0000555556896e27 in ui::GetSupportedScaleFactor(float) () #1 0x0000555558fc3e37 in libgtk2ui::Gtk2UI::GetDeviceScaleFactor() const () #2 0x0000555558fc2ee8 in libgtk2ui::Gtk2UI::UpdateDefaultFont(_PangoFontDescription const*) () #3 0x0000555558fc09a5 in libgtk2ui::Gtk2UI::LoadGtkValues() () #4 0x0000555558fc073d in libgtk2ui::Gtk2UI::Initialize() () #5 0x00005555581b0860 in ChromeBrowserMainExtraPartsAura::ToolkitInitialized() () #6 0x0000555556283efa in ChromeBrowserMainParts::ToolkitInitialized() () #7 0x00005555586a30de in content::BrowserMainLoop::InitializeToolkit() () #8 0x00005555584b9760 in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) () #9 0x00005555584b947d in content::BrowserMain(content::MainFunctionParams const&) () #10 0x00005555565053d5 in content::ContentMainRunnerImpl::Run() () #11 0x0000555556504170 in content::ContentMain(content::ContentMainParams const&) () #12 0x0000555555ffe9f8 in ChromeMain () #13 0x00007ffff1523fe0 in __libc_start_main () at /lib64/libc.so.6 #14 0x0000555555ffe8af in _start () https://codereview.chromium.org/1070433002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/1070433002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:284: { On 2015/04/09 16:47:27, Elliot Glaysher wrote: > Is there a reason this is in a block instead of just float device_scale_factor = > views::LinuxUI::instance()->GetDeviceScaleFactor()? > > (It looks like you started out with a temp vector in here, but now that that's > gone, there's no reason to have this scope.) Done.
On 2015/04/09 18:11:15, stapelberg wrote: > On 2015/04/09 17:22:23, oshima wrote: > > > https://codereview.chromium.org/1070433002/diff/20001/chrome/browser/ui/libgt... > > File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right): > > > > > https://codereview.chromium.org/1070433002/diff/20001/chrome/browser/ui/libgt... > > chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1430: return > > ui::GetScaleForScaleFactor(ui::GetSupportedScaleFactor(scale)); > > On 2015/04/09 06:49:34, stapelberg wrote: > > > On 2015/04/08 23:57:41, oshima wrote: > > > > you may want to add more scales for linux (133 150 etc). > > > > > > Yeah, I’ll investigate that in a separate CL so that we can revert them > > > separately if necessary. > > > > I think my comment was a bit confusing. Technically, you can just pass scale, > > although we don't want to allow arbitrary scale factor. On Linux, we can pick > > something like 1.25, 1.5 and 2.0 and use closest scale factor. (ChromeOS > > currently support 1.0, 1.25 and 2.0) > > I’m not sure I understand what you’re saying. Currently, we are choosing either > 1.0 or 2.0 on Linux because these are the only supported scale factors. If you > want me to add more scale factors then I’d like to prefer to do that in a > separate CL as explained in my previous comment. "Supported" simply means the resource for the scale factor is available. You still can use other scale factor and chrome will scale the image for you. I have a plan to rename it as it's confusing. Yes, please do it in separate CL.
On 2015/04/09 18:11:29, stapelberg wrote: > PTAL. > > https://codereview.chromium.org/1070433002/diff/40001/ui/views/linux_ui/linux... > File ui/views/linux_ui/linux_ui.h (right): > > https://codereview.chromium.org/1070433002/diff/40001/ui/views/linux_ui/linux... > ui/views/linux_ui/linux_ui.h:156: virtual void UpdateDeviceScaleFactor(float > device_scale_factor) = 0; > On 2015/04/09 16:50:01, sadrul wrote: > > Given that LinuxUI is now responsible for getting the DSF (which is used to > set > > the DSF on the display), do we still need UpdateDeviceScaleFactor()? > > Yes. If I try to call GetDeviceScaleFactor() directly in UpdateDefaultFont(), I > get a segfault because the supported scale factors have not yet been initialized > (we’ve had the same thing in another CL a while ago): You should be able to remove this if you implement what I suggested to support more scale factors. (in a separate CL) > > #0 0x0000555556896e27 in ui::GetSupportedScaleFactor(float) () > #1 0x0000555558fc3e37 in libgtk2ui::Gtk2UI::GetDeviceScaleFactor() const () > #2 0x0000555558fc2ee8 in > libgtk2ui::Gtk2UI::UpdateDefaultFont(_PangoFontDescription const*) () > #3 0x0000555558fc09a5 in libgtk2ui::Gtk2UI::LoadGtkValues() () > #4 0x0000555558fc073d in libgtk2ui::Gtk2UI::Initialize() () > #5 0x00005555581b0860 in ChromeBrowserMainExtraPartsAura::ToolkitInitialized() > () > #6 0x0000555556283efa in ChromeBrowserMainParts::ToolkitInitialized() () > #7 0x00005555586a30de in content::BrowserMainLoop::InitializeToolkit() () > #8 0x00005555584b9760 in > content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) > () > #9 0x00005555584b947d in content::BrowserMain(content::MainFunctionParams > const&) () > #10 0x00005555565053d5 in content::ContentMainRunnerImpl::Run() () > #11 0x0000555556504170 in content::ContentMain(content::ContentMainParams > const&) () > #12 0x0000555555ffe9f8 in ChromeMain () > #13 0x00007ffff1523fe0 in __libc_start_main () at /lib64/libc.so.6 > #14 0x0000555555ffe8af in _start () > > https://codereview.chromium.org/1070433002/diff/40001/ui/views/widget/desktop... > File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): > > https://codereview.chromium.org/1070433002/diff/40001/ui/views/widget/desktop... > ui/views/widget/desktop_aura/desktop_screen_x11.cc:284: { > On 2015/04/09 16:47:27, Elliot Glaysher wrote: > > Is there a reason this is in a block instead of just float device_scale_factor > = > > views::LinuxUI::instance()->GetDeviceScaleFactor()? > > > > (It looks like you started out with a temp vector in here, but now that that's > > gone, there's no reason to have this scope.) > > Done.
erg, friendly ping? I’d like to get this in ASAP so that the next chrome unstable release fixes the issue for the affected users. Thanks.
lgtm
The CQ bit was checked by stapelberg@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1070433002/#ps60001 (title: "device scale factor detection: use gtk-xft-dpi consistently")
The CQ bit was unchecked by stapelberg@google.com
The CQ bit was checked by stapelberg@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1070433002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by stapelberg@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, erg@chromium.org Link to the patchset: https://codereview.chromium.org/1070433002/#ps80001 (title: "device scale factor detection: use gtk-xft-dpi consistently")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1070433002/80001
The CQ bit was checked by stapelberg@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, erg@chromium.org Link to the patchset: https://codereview.chromium.org/1070433002/#ps100001 (title: "device scale factor detection: use gtk-xft-dpi consistently")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1070433002/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/06cda64742b45e1eb10bbbb66bca4b4da2efb3c9 Cr-Commit-Position: refs/heads/master@{#324782}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1079883002/ by timloh@chromium.org. The reason for reverting is: This patch makes content_shell segfault during startup. Program received signal SIGSEGV, Segmentation fault. 0x00000000049d190a in views::DesktopScreenX11::BuildDisplaysFromXRandRInfo (this=0x35ed099f9200) at ../../ui/views/widget/desktop_aura/desktop_screen_x11.cc:291 291 views::LinuxUI::instance()->GetDeviceScaleFactor(); (gdb) bt #0 0x00000000049d190a in views::DesktopScreenX11::BuildDisplaysFromXRandRInfo (this=0x35ed099f9200) at ../../ui/views/widget/desktop_aura/desktop_screen_x11.cc:291 #1 0x00000000049d096f in views::DesktopScreenX11::DesktopScreenX11 (this=0x35ed099f9200) at ../../ui/views/widget/desktop_aura/desktop_screen_x11.cc:107 #2 0x00000000049d1fab in views::CreateDesktopScreen () at ../../ui/views/widget/desktop_aura/desktop_screen_x11.cc:370 #3 0x0000000000451f28 in content::Shell::PlatformInitialize (default_window_size=...) at ../../content/shell/browser/shell_views.cc:421 #4 0x0000000000440158 in content::Shell::Initialize () at ../../content/shell/browser/shell.cc:146. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
