Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
153403003:
Move supports-high-dpi flag into registry. (Closed)
DescriptionMove supports-high-dpi flag into registry.
Calls to SetProcessDpiAwareness need to happen immediately when the app starts. Specifically, before user profile settings have been initialized.
This patch moves the --supports-high-dpi into the registry.
BUG=339152, 149881, 160457
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256811
Patch Set 1 #Patch Set 2 : Clean #
Total comments: 3
Patch Set 3 : Remove flag from profile. #Patch Set 4 : Code cleanup - remove useless comment. #
Total comments: 12
Patch Set 5 : Reformat, remove strings #
Total comments: 2
Patch Set 6 : Move constants to top of namespace. #Patch Set 7 : Adding testing support for high-dpi setting. #Patch Set 8 : Cleanup. Renaming function. #
Messages
Total messages: 35 (0 generated)
What is it about high DPI support that makes it need to be initialized early on? Or what are we doing by the time we get to profile reading that is too late? https://codereview.chromium.org/153403003/diff/220001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/153403003/diff/220001/chrome/browser/about_fl... chrome/browser/about_flags.cc:84: std::wstring ConvertToWString(const std::string& s) { There is a bunch of code for reading/writing to the registry in base/win/registry.
I don't like the complexity this adds, along with the nest of ifdefs for only one flag. I propose we tell folks that want to use this they have to pass in a switch or modify a registry if we want that.
In Win8.1, SetProcessDpiAwareness needs to be called before any calls that depend on it (which is pretty much anything that queries the system, as far as I can tell.) This was noted in http://crbug.com/330093. https://codereview.chromium.org/100923011 changed the location of the call to EnableHighDPISupport. https://codereview.chromium.org/153403003/diff/220001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/153403003/diff/220001/chrome/browser/about_fl... chrome/browser/about_flags.cc:84: std::wstring ConvertToWString(const std::string& s) { On 2014/02/11 14:57:00, sky wrote: > There is a bunch of code for reading/writing to the registry in > base/win/registry. I'm using those functions below. Those functions (base::win::RegKey) use wstring's, so this function helps with conversions.
On 2014/02/11 17:09:01, sky wrote: > I don't like the complexity this adds, along with the nest of ifdefs for only > one flag. I propose we tell folks that want to use this they have to pass in a > switch or modify a registry if we want that. I agree that this is kind of ugly. I could refactor to reduce the number of ifdef's, and possibly move the registry gunk into a separate file. Would that help? I'd also be happy to own the "tear out this stuff after highdpi becomes standard" bug. Removing this setting from about://flags would greatly reduce the number of testers.
https://codereview.chromium.org/153403003/diff/220001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/153403003/diff/220001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2238: DWORD registry_value = ReadRegistryValue( Self-review: This logic depends on other code that is #if defined(OS_WINDOWS). I'll fix this.
What's so hard about passing in a switch or setting the registry entry? I agree it reduces the number of users that may use it, but getting random Joe's using a flag isn't the main intention with about:flags. I also think shoe horning high DPI into a flag is equally weird. For example, if you open multiple profiles the flag only makes sense on the first profile. -Scott On Tue, Feb 11, 2014 at 12:28 PM, <girard@chromium.org> wrote: > On 2014/02/11 17:09:01, sky wrote: >> >> I don't like the complexity this adds, along with the nest of ifdefs for >> only >> one flag. I propose we tell folks that want to use this they have to pass >> in a >> switch or modify a registry if we want that. > > > I agree that this is kind of ugly. I could refactor to reduce the number of > ifdef's, and possibly move the registry gunk into a separate file. Would > that > help? I'd also be happy to own the "tear out this stuff after highdpi > becomes > standard" bug. > > Removing this setting from about://flags would greatly reduce the number of > testers. > > https://codereview.chromium.org/153403003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/11 21:57:37, sky wrote: > What's so hard about passing in a switch or setting the registry > entry? I agree it reduces the number of users that may use it, but > getting random Joe's using a flag isn't the main intention with > about:flags. I also think shoe horning high DPI into a flag is equally > weird. For example, if you open multiple profiles the flag only makes > sense on the first profile. > > -Scott > > On Tue, Feb 11, 2014 at 12:28 PM, <mailto:girard@chromium.org> wrote: > > On 2014/02/11 17:09:01, sky wrote: > >> > >> I don't like the complexity this adds, along with the nest of ifdefs for > >> only > >> one flag. I propose we tell folks that want to use this they have to pass > >> in a > >> switch or modify a registry if we want that. > > > > > > I agree that this is kind of ugly. I could refactor to reduce the number of > > ifdef's, and possibly move the registry gunk into a separate file. Would > > that > > help? I'd also be happy to own the "tear out this stuff after highdpi > > becomes > > standard" bug. > > > > Removing this setting from about://flags would greatly reduce the number of > > testers. > > > > https://codereview.chromium.org/153403003/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. [Sorry for the delay... didn't receive an email on your last review.] Using a command-line switch under win/ash is risky/troublesome according to cpu/zturner, and I agree. I'm bumping the flag into the registry, and will expect testers to set it using regedit or equivalent.
The current patch removes the flag from about://flags, as requested. PTAL.
just style nits https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc File ui/gfx/win/dpi.cc (right): https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:104: DWORD ReadRegistryValue(HKEY root, const wchar_t* base_key, nit: when you wrap to multiple lines, one parameter per line. https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:106: base::win::RegKey regKey(HKEY_CURRENT_USER, reg_key https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:107: base_key, nit: spacing https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:111: regKey.ReadValueDW(value_name,&value) == ERROR_SUCCESS) { 'name,&'-> 'name, &' https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:161: HKEY_CURRENT_USER,kRegistryProfilePath, nit: spaces after ',' https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:172: if (!SetProcessDpiAwarenessWrapper(PROCESS_SYSTEM_DPI_AWARE)) { nit: combine ifs.
Cleanup up formatting/nits. Also removed unused strings. https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc File ui/gfx/win/dpi.cc (right): https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:104: DWORD ReadRegistryValue(HKEY root, const wchar_t* base_key, On 2014/02/19 21:20:32, sky wrote: > nit: when you wrap to multiple lines, one parameter per line. Done. https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:106: base::win::RegKey regKey(HKEY_CURRENT_USER, On 2014/02/19 21:20:32, sky wrote: > reg_key Done. https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:107: base_key, On 2014/02/19 21:20:32, sky wrote: > nit: spacing Done. https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:111: regKey.ReadValueDW(value_name,&value) == ERROR_SUCCESS) { On 2014/02/19 21:20:32, sky wrote: > 'name,&'-> 'name, &' Done. https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:161: HKEY_CURRENT_USER,kRegistryProfilePath, On 2014/02/19 21:20:32, sky wrote: > nit: spaces after ',' Done. https://codereview.chromium.org/153403003/diff/340001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:172: if (!SetProcessDpiAwarenessWrapper(PROCESS_SYSTEM_DPI_AWARE)) { On 2014/02/19 21:20:32, sky wrote: > nit: combine ifs. Done.
LGTM https://codereview.chromium.org/153403003/diff/400001/ui/gfx/win/dpi.cc File ui/gfx/win/dpi.cc (right): https://codereview.chromium.org/153403003/diff/400001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:101: const wchar_t kRegistryProfilePath[] = L"SOFTWARE\\Google\\Chrome\\Profile"; nit: move constants to top of namespace (right here doesn't make sense since this function can take any value).
Thanks! https://codereview.chromium.org/153403003/diff/400001/ui/gfx/win/dpi.cc File ui/gfx/win/dpi.cc (right): https://codereview.chromium.org/153403003/diff/400001/ui/gfx/win/dpi.cc#newco... ui/gfx/win/dpi.cc:101: const wchar_t kRegistryProfilePath[] = L"SOFTWARE\\Google\\Chrome\\Profile"; On 2014/02/20 15:48:43, sky wrote: > nit: move constants to top of namespace (right here doesn't make sense since > this function can take any value). Done.
The CQ bit was checked by girard@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/girard@chromium.org/153403003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests, content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by girard@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/girard@chromium.org/153403003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Scott, can you take another look at this? Since the LGTM, I've added a fix to correct a unit test failure. Specifically, I've added ForceHighDPISupportForTesting(float scale)
LGTM
The CQ bit was checked by girard@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/girard@chromium.org/153403003/830001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by girard@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/girard@chromium.org/153403003/830001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_clang_dbg
The CQ bit was checked by girard@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/girard@chromium.org/153403003/830001
On 2014/03/13 04:36:54, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > tryserver.chromium on linux_chromium_chromeos_clang_dbg Looks like this patch is getting blocked by https://code.google.com/p/chromium/issues/detail?id=351672 Fixed 7 hours ago. Letès try again.
Message was sent while issue was closed.
Change committed as 256811 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
