Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1563183008:
Added capability on Windows to get the physical dimensions of your attached monitors. Also added th… (Closed)
DescriptionAdded capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing.
BUG=547914
TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors
Committed: https://crrev.com/b840e2f4ec9132b931f7ad54fd8fd20285e4eaa3
Cr-Commit-Position: refs/heads/master@{#369635}
Committed: https://crrev.com/b2f706f91654deb8ffea59a2be4bdc38c1f2bb7f
Cr-Commit-Position: refs/heads/master@{#370180}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 40
Patch Set 4 : addressed comments #Patch Set 5 : . #
Total comments: 16
Patch Set 6 : review comments 2 #
Total comments: 35
Patch Set 7 : reivew comments 3 #
Total comments: 2
Patch Set 8 : . #
Total comments: 6
Patch Set 9 : . #
Total comments: 9
Patch Set 10 : . #Patch Set 11 : . #
Total comments: 5
Patch Set 12 : fix gn build and applied guid brace warning fix #
Messages
Total messages: 56 (14 generated)
Description was changed from ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors ========== to ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors ==========
bsep@chromium.org changed reviewers: + robliao@chromium.org, scottmg@chromium.org
Cool, looks good. Just some minor stuff. https://codereview.chromium.org/1563183008/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1563183008/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:163: std::ostringstream size_stream; I see we've recently changed the style guide to allow streams (!!), but other parts of this file and most of Chrome use base::StringPrintf, so I think it'd be better to stick to that here. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/display.cc File ui/gfx/display.cc (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/display.cc#newco... ui/gfx/display.cc:208: for (size_t i = 0; i < display_sizes.size(); ++i) { for (const auto& display_size : display_sizes) ? https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/display.cc#newco... ui/gfx/display.cc:220: // unimplemented "Unimplemented." or just delete this line, since that's kind of what NOTREACHED() means. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/screen_win.h File ui/gfx/screen_win.h (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/screen_win.h#new... ui/gfx/screen_win.h:32: static int64_t GenerateDisplayId(const std::wstring& str); #include <string> for std::wstring. It looks like you're mixing some string16 with wstring here. Maybe it should take std::string here, and have the caller do either WideToUTF8 or UTF16ToUTF8 depending on what it's got. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. I think there's not supposed to be any "(c)" any more. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:15: const GUID GUID_DEVICEINTERFACE_MONITOR = { Any documentation URL you could add for this magic value? https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:36: TCHAR value_name[256]; We don't generally use TCHAR, _t, etc., just wchar_t and wcscmp(), etc. below is probably more normal. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:44: while (ERROR_SUCCESS == RegEnumValue(reg_key, reg_value_index, &value_name[0], Can you use base/win/registry.h RegKey for this? ~RegKey makes sure the RegCloseKey() doesn't get lost. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:45: &value_name_length, NULL, &data_type, NULL -> nullptr throughout. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:53: // 68: 4 bits for each of height and width (respectively) most "height and width (respectively)" sounds to me like it'd be 0xf0 for height, and 0x0f for width. Code correct as is? If so, maybe remove the respectively, since it's clear which would is which. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:57: found = true; Break here? https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:75: (PDWORD)&buffer_size, device_info); No c-style casts. I think buffer_size can just be a DWORD here and remove the cast entirely? https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:75: (PDWORD)&buffer_size, device_info); Check that this returns false and GetLastError() is as expected. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:81: NULL); And that this one succeeds. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:91: // get a handle to the list of device interfaces that are monitors via Setup Capitals start and periods at the end of comments/sentences throughout.
https://codereview.chromium.org/1563183008/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1563183008/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:156: for (size_t i = 0; i < display_sizes.size(); ++i) { Could probably use the C++11 foreach here too. for (const auto& display_size : display_sizes) https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/screen_win.h File ui/gfx/screen_win.h (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/screen_win.h#new... ui/gfx/screen_win.h:32: static int64_t GenerateDisplayId(const std::wstring& str); On 2016/01/09 01:00:05, scottmg wrote: > #include <string> for std::wstring. > > It looks like you're mixing some string16 with wstring here. Maybe it should > take std::string here, and have the caller do either WideToUTF8 or UTF16ToUTF8 > depending on what it's got. Alternatively, you can do... static int64_t HashDeviceName(const wchar_t* device_name); This is a similar change I've got in an in progress CL. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:88: void GetPhysicalSizeForDisplays(std::vector<DisplaySize>* out) { Can this be part of ScreenWin Display generation? https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_size.h File ui/gfx/win/physical_size.h (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Follow the guidance regarding this header from https://www.chromium.org/developers/coding-style#TOC-File-headers https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.h:15: struct DisplaySize { Maybe PhysicalDisplaySize?
https://codereview.chromium.org/1563183008/diff/40001/content/browser/gpu/gpu... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1563183008/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:156: for (size_t i = 0; i < display_sizes.size(); ++i) { On 2016/01/11 18:42:20, robliao wrote: > Could probably use the C++11 foreach here too. > for (const auto& display_size : display_sizes) Done. https://codereview.chromium.org/1563183008/diff/40001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:163: std::ostringstream size_stream; On 2016/01/09 01:00:05, scottmg wrote: > I see we've recently changed the style guide to allow streams (!!), but other > parts of this file and most of Chrome use base::StringPrintf, so I think it'd be > better to stick to that here. Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/display.cc File ui/gfx/display.cc (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/display.cc#newco... ui/gfx/display.cc:208: for (size_t i = 0; i < display_sizes.size(); ++i) { On 2016/01/09 01:00:05, scottmg wrote: > for (const auto& display_size : display_sizes) ? Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/display.cc#newco... ui/gfx/display.cc:220: // unimplemented On 2016/01/09 01:00:05, scottmg wrote: > "Unimplemented." or just delete this line, since that's kind of what > NOTREACHED() means. Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/screen_win.h File ui/gfx/screen_win.h (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/screen_win.h#new... ui/gfx/screen_win.h:32: static int64_t GenerateDisplayId(const std::wstring& str); On 2016/01/11 18:42:20, robliao wrote: > On 2016/01/09 01:00:05, scottmg wrote: > > #include <string> for std::wstring. > > > > It looks like you're mixing some string16 with wstring here. Maybe it should > > take std::string here, and have the caller do either WideToUTF8 or UTF16ToUTF8 > > depending on what it's got. > > Alternatively, you can do... > static int64_t HashDeviceName(const wchar_t* device_name); > > This is a similar change I've got in an in progress CL. Done. I went with Scott's suggestion, since display.cc is not dealing with wchar_t* here. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/01/09 01:00:05, scottmg wrote: > I think there's not supposed to be any "(c)" any more. Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:15: const GUID GUID_DEVICEINTERFACE_MONITOR = { On 2016/01/09 01:00:05, scottmg wrote: > Any documentation URL you could add for this magic value? Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:36: TCHAR value_name[256]; On 2016/01/09 01:00:05, scottmg wrote: > We don't generally use TCHAR, _t, etc., just wchar_t and wcscmp(), etc. below is > probably more normal. Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:44: while (ERROR_SUCCESS == RegEnumValue(reg_key, reg_value_index, &value_name[0], On 2016/01/09 01:00:05, scottmg wrote: > Can you use base/win/registry.h RegKey for this? ~RegKey makes sure the > RegCloseKey() doesn't get lost. Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:45: &value_name_length, NULL, &data_type, On 2016/01/09 01:00:06, scottmg wrote: > NULL -> nullptr throughout. Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:53: // 68: 4 bits for each of height and width (respectively) most On 2016/01/09 01:00:05, scottmg wrote: > "height and width (respectively)" sounds to me like it'd be 0xf0 for height, and > 0x0f for width. Code correct as is? If so, maybe remove the respectively, since > it's clear which would is which. Yeah that's confusing. I'll just let the bitmask speak for itself. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:57: found = true; On 2016/01/09 01:00:05, scottmg wrote: > Break here? Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:75: (PDWORD)&buffer_size, device_info); On 2016/01/09 01:00:06, scottmg wrote: > No c-style casts. I think buffer_size can just be a DWORD here and remove the > cast entirely? Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:75: (PDWORD)&buffer_size, device_info); On 2016/01/09 01:00:05, scottmg wrote: > Check that this returns false and GetLastError() is as expected. Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:81: NULL); On 2016/01/09 01:00:05, scottmg wrote: > And that this one succeeds. Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:88: void GetPhysicalSizeForDisplays(std::vector<DisplaySize>* out) { On 2016/01/11 18:42:20, robliao wrote: > Can this be part of ScreenWin Display generation? It could, but I'm not clear on what the advantage of that would be. I don't think most clients of Display care about the physical size? This is also easier to call for the about:gpu page, since it doesn't have the Display objects easily accessible. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:91: // get a handle to the list of device interfaces that are monitors via Setup On 2016/01/09 01:00:05, scottmg wrote: > Capitals start and periods at the end of comments/sentences throughout. Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_size.h File ui/gfx/win/physical_size.h (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/01/11 18:42:20, robliao wrote: > Follow the guidance regarding this header from > https://www.chromium.org/developers/coding-style#TOC-File-headers Done. https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.h:15: struct DisplaySize { On 2016/01/11 18:42:20, robliao wrote: > Maybe PhysicalDisplaySize? Done.
Do you also happen to know under what circumstances physical display information is unavailable? https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:88: void GetPhysicalSizeForDisplays(std::vector<DisplaySize>* out) { On 2016/01/12 00:23:47, Bret Sepulveda wrote: > On 2016/01/11 18:42:20, robliao wrote: > > Can this be part of ScreenWin Display generation? > > It could, but I'm not clear on what the advantage of that would be. I don't > think most clients of Display care about the physical size? This is also easier > to call for the about:gpu page, since it doesn't have the Display objects easily > accessible. Most, if not all calls to Display are quick cached value lookups. All of the computation happens during the generation of the display. This would be following that pattern. This would also allow you to update the getter to something more display-like like gfx::Size GetPhysicalSize()
(non-owner) LGTM. https://codereview.chromium.org/1563183008/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1563183008/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:158: double size_mm = sqrt(w * w + h * h); Optional nit: Since these are just breaking up a calculation, maybe "const int ...", and "const double ..." would be nice here. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:31: SP_DEVINFO_DATA& device_info, No non-const references. If it can be a const ref, that's better, or otherwise, pass it as a pointer if it needs to be mutable. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:62: SP_DEVICE_INTERFACE_DATA& interface_data, Same. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:145: height_mm); nit; I'd probably just put this inline into the push_back as out->push_back(PhysicalDisplaySize(...)); rather than making a named temporary. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:148: break; Should this be inside "if (found)"? https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_size.h File ui/gfx/win/physical_size.h (right): https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.h:20: PhysicalDisplaySize(std::string display_name, int width_mm, int height_mm) Always pass std::string as "const std::string&". https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.h:25: void GFX_EXPORT GFX_EXPORT before void, not after. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.h:26: GetPhysicalSizeForDisplays(std::vector<PhysicalDisplaySize>* out); Consider having this return std::vector<PhysicalDisplaySize> as it seems like that'd be more natural.
On 2016/01/12 01:00:37, robliao wrote: > Do you also happen to know under what circumstances physical display information > is unavailable? > On a headless system obviously, but otherwise I'm not sure. If you're using a display device that doesn't have an EDID block, for example a VM maybe, but all actual monitors are SUPPOSED to have one. > https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... > File ui/gfx/win/physical_size.cc (right): > > https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... > ui/gfx/win/physical_size.cc:88: void > GetPhysicalSizeForDisplays(std::vector<DisplaySize>* out) { > On 2016/01/12 00:23:47, Bret Sepulveda wrote: > > On 2016/01/11 18:42:20, robliao wrote: > > > Can this be part of ScreenWin Display generation? > > > > It could, but I'm not clear on what the advantage of that would be. I don't > > think most clients of Display care about the physical size? This is also > easier > > to call for the about:gpu page, since it doesn't have the Display objects > easily > > accessible. > > Most, if not all calls to Display are quick cached value lookups. All of the > computation happens during the generation of the display. This would be > following that pattern. > > This would also allow you to update the getter to something more display-like > like > gfx::Size GetPhysicalSize() I can change the getter regardless, if you think it makes more sense. And if you're saying you'd prefer the eager initialization I can change that too.
On 2016/01/12 01:11:35, Bret Sepulveda wrote: > On 2016/01/12 01:00:37, robliao wrote: > > Do you also happen to know under what circumstances physical display > information > > is unavailable? > > > > On a headless system obviously, but otherwise I'm not sure. If you're using a > display device that doesn't have an EDID block, for example a VM maybe, but all > actual monitors are SUPPOSED to have one. VGA rather than >= DVI too I guess. > > > > https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... > > File ui/gfx/win/physical_size.cc (right): > > > > > https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... > > ui/gfx/win/physical_size.cc:88: void > > GetPhysicalSizeForDisplays(std::vector<DisplaySize>* out) { > > On 2016/01/12 00:23:47, Bret Sepulveda wrote: > > > On 2016/01/11 18:42:20, robliao wrote: > > > > Can this be part of ScreenWin Display generation? > > > > > > It could, but I'm not clear on what the advantage of that would be. I don't > > > think most clients of Display care about the physical size? This is also > > easier > > > to call for the about:gpu page, since it doesn't have the Display objects > > easily > > > accessible. > > > > Most, if not all calls to Display are quick cached value lookups. All of the > > computation happens during the generation of the display. This would be > > following that pattern. > > > > This would also allow you to update the getter to something more display-like > > like > > gfx::Size GetPhysicalSize() > > I can change the getter regardless, if you think it makes more sense. And if > you're saying you'd prefer the eager initialization I can change that too.
On 2016/01/12 01:13:18, scottmg wrote: > On 2016/01/12 01:11:35, Bret Sepulveda wrote: > > On 2016/01/12 01:00:37, robliao wrote: > > > Do you also happen to know under what circumstances physical display > > information > > > is unavailable? > > > > > > > On a headless system obviously, but otherwise I'm not sure. If you're using a > > display device that doesn't have an EDID block, for example a VM maybe, but > all > > actual monitors are SUPPOSED to have one. > > VGA rather than >= DVI too I guess. > > > > > > > > > https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... > > > File ui/gfx/win/physical_size.cc (right): > > > > > > > > > https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... > > > ui/gfx/win/physical_size.cc:88: void > > > GetPhysicalSizeForDisplays(std::vector<DisplaySize>* out) { > > > On 2016/01/12 00:23:47, Bret Sepulveda wrote: > > > > On 2016/01/11 18:42:20, robliao wrote: > > > > > Can this be part of ScreenWin Display generation? > > > > > > > > It could, but I'm not clear on what the advantage of that would be. I > don't > > > > think most clients of Display care about the physical size? This is also > > > easier > > > > to call for the about:gpu page, since it doesn't have the Display objects > > > easily > > > > accessible. > > > > > > Most, if not all calls to Display are quick cached value lookups. All of the > > > computation happens during the generation of the display. This would be > > > following that pattern. > > > > > > This would also allow you to update the getter to something more > display-like > > > like > > > gfx::Size GetPhysicalSize() > > > > I can change the getter regardless, if you think it makes more sense. And if > > you're saying you'd prefer the eager initialization I can change that too. We should have no Displays on headless systems, so we should be good to go there :-) I'm more interested in what callers should do when physical monitor dimensions are unavailable. MSDN claims that EDIDs should always be available (analog or digital). https://msdn.microsoft.com/en-us/library/windows/hardware/jj133967(v=vs.85) I guess the driver provides some of this data for VGA monitors then?
On 2016/01/12 01:26:04, robliao wrote: > On 2016/01/12 01:13:18, scottmg wrote: > > On 2016/01/12 01:11:35, Bret Sepulveda wrote: > > > On 2016/01/12 01:00:37, robliao wrote: > > > > Do you also happen to know under what circumstances physical display > > > information > > > > is unavailable? > > > > > > > > > > On a headless system obviously, but otherwise I'm not sure. If you're using > a > > > display device that doesn't have an EDID block, for example a VM maybe, but > > all > > > actual monitors are SUPPOSED to have one. > > > > VGA rather than >= DVI too I guess. > > > > > > > > > > > > > > > https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... > > > > File ui/gfx/win/physical_size.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1563183008/diff/40001/ui/gfx/win/physical_siz... > > > > ui/gfx/win/physical_size.cc:88: void > > > > GetPhysicalSizeForDisplays(std::vector<DisplaySize>* out) { > > > > On 2016/01/12 00:23:47, Bret Sepulveda wrote: > > > > > On 2016/01/11 18:42:20, robliao wrote: > > > > > > Can this be part of ScreenWin Display generation? > > > > > > > > > > It could, but I'm not clear on what the advantage of that would be. I > > don't > > > > > think most clients of Display care about the physical size? This is also > > > > easier > > > > > to call for the about:gpu page, since it doesn't have the Display > objects > > > > easily > > > > > accessible. > > > > > > > > Most, if not all calls to Display are quick cached value lookups. All of > the > > > > computation happens during the generation of the display. This would be > > > > following that pattern. > > > > > > > > This would also allow you to update the getter to something more > > display-like > > > > like > > > > gfx::Size GetPhysicalSize() > > > > > > I can change the getter regardless, if you think it makes more sense. And if > > > you're saying you'd prefer the eager initialization I can change that too. > > We should have no Displays on headless systems, so we should be good to go there > :-) > > I'm more interested in what callers should do when physical monitor dimensions > are unavailable. > MSDN claims that EDIDs should always be available (analog or digital). > https://msdn.microsoft.com/en-us/library/windows/hardware/jj133967(v=vs.85) > I guess the driver provides some of this data for VGA monitors then? As for > I can change the getter regardless, if you think it makes more sense. And if > you're saying you'd prefer the eager initialization I can change that too. Yup, make the changes. This brings this more in line with what Display offers. Thanks!
made the display.h method changes as well https://codereview.chromium.org/1563183008/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1563183008/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:158: double size_mm = sqrt(w * w + h * h); On 2016/01/12 01:03:13, scottmg wrote: > Optional nit: Since these are just breaking up a calculation, maybe "const int > ...", and "const double ..." would be nice here. Done. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:31: SP_DEVINFO_DATA& device_info, On 2016/01/12 01:03:13, scottmg wrote: > No non-const references. If it can be a const ref, that's better, or otherwise, > pass it as a pointer if it needs to be mutable. Done. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:62: SP_DEVICE_INTERFACE_DATA& interface_data, On 2016/01/12 01:03:13, scottmg wrote: > Same. Done. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:145: height_mm); On 2016/01/12 01:03:13, scottmg wrote: > nit; I'd probably just put this inline into the push_back as > > out->push_back(PhysicalDisplaySize(...)); > > rather than making a named temporary. Done. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.cc:148: break; On 2016/01/12 01:03:13, scottmg wrote: > Should this be inside "if (found)"? No, the Device ID should be unique, so if it matches the Setup Device Path there's no reason to continue looking even if getting the physical size fails. Ideally we'd continue the outermost loop instead but that'd look even more confusing. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_size.h File ui/gfx/win/physical_size.h (right): https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.h:20: PhysicalDisplaySize(std::string display_name, int width_mm, int height_mm) On 2016/01/12 01:03:13, scottmg wrote: > Always pass std::string as "const std::string&". Done. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.h:25: void GFX_EXPORT On 2016/01/12 01:03:13, scottmg wrote: > GFX_EXPORT before void, not after. Done. https://codereview.chromium.org/1563183008/diff/80001/ui/gfx/win/physical_siz... ui/gfx/win/physical_size.h:26: GetPhysicalSizeForDisplays(std::vector<PhysicalDisplaySize>* out); On 2016/01/12 01:03:13, scottmg wrote: > Consider having this return std::vector<PhysicalDisplaySize> as it seems like > that'd be more natural. Done.
Thanks for the changes! Did a closer reading and we're almost good to go. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/display.h File ui/gfx/display.h (right): https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/display.h#newco... ui/gfx/display.h:143: void SetPhysicalSize(int width_mm, int height_mm); SetPhysicalSizeMm(gfx::Size size_mm) for symmetry to GetPhysicalSizeMm. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/display.h#newco... ui/gfx/display.h:162: int physical_width_mm_; gfx::Size physical_size_mm; https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/screen_win.h File ui/gfx/screen_win.h (right): https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/screen_win.h#ne... ui/gfx/screen_win.h:8: #include <stdint.h> Move this include to screen_win.cc https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:5: #include "ui/gfx/win/physical_size.h" https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes #include "ui/gfx/win/physical_size.h" #include <windows.h> (Exception to the rule since things tend to break if it isn't first) #include <setupapi.h> #include <iostream> #include "base/logging.h" #include "base/memory/scoped_ptr.h" #include "base/scoped_generic.h" #include "base/strings/utf_string_conversions.h" #include "base/win/registry.h" https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:18: // https://msdn.microsoft.com/en-us/library/windows/hardware/ff545901.aspx Nit: Add comment: // {E6F07B5F-EE97-4A90-B076-33F57BF4EAA7} https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:35: device_info_list, device_info, DICS_FLAG_GLOBAL, 0, DIREG_DEV, KEY_READ)); The preference in ui/gfx/* seems to be to line up the arguments when the argument list exceeds the 80 char limit, falling back to this format when single argument lined up this way exceeds the 80 char limit. base::win::RegKey reg_key(SetupDiOpenDevRegKey(device_info_list, device_info, DICS_FLAG_GLOBAL, 0, DIREG_DEV, KEY_READ)); fits. Similar rules apply to assignments: type var_name = func_call(arg1, arg2, arg3) (Only if arg1, arg2, arg3 would exceed the 80 char limit) Apply to the rest of the file. Personally, I prefer what you have right now, but we follow whatever is already established in this section. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:65: device_info->cbSize = sizeof(SP_DEVINFO_DATA); Similar to Windows, the caller should be initializing this value. You can, however, DCHECK(device_info->cbSize == sizeof(*device_info)) https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:71: if (success || GetLastError() != ERROR_INSUFFICIENT_BUFFER) { As set up, this call is guaranteed to fail, so a check for success is really optional here. However, feel free to keep the ERROR_INSUFFICIENT_BUFFER check. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:91: // Get a handle to the list of device interfaces that are monitors via Setup. Remove this comment. See https://google.github.io/styleguide/cppguide.html#Comments under "Don'ts". Apply to the rest of the file. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:108: SP_DEVINFO_DATA device_info = {0}; cbSize should be set up here as part of initialization. device_info.cbSize = sizeof(device_info); Nit: {} https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:111: bool get_info_succeeded = Optional: You could plausibly inline this into the "if" (same with "found" below). https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:119: DISPLAY_DEVICE display_device = {0}; Nit: {} https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:120: display_device.cb = sizeof(DISPLAY_DEVICE); Nit: sizeof(display_device) https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:127: DISPLAY_DEVICE attached_device = {0}; Nit: {} https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:128: attached_device.cb = sizeof(DISPLAY_DEVICE); Nit: sizeof(attached_device) https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:136: // We've found the monitor that matches the Setup device. Nit: Setup -> setup.
https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/display.h File ui/gfx/display.h (right): https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/display.h#newco... ui/gfx/display.h:143: void SetPhysicalSize(int width_mm, int height_mm); On 2016/01/13 00:22:10, robliao wrote: > SetPhysicalSizeMm(gfx::Size size_mm) for symmetry to GetPhysicalSizeMm. Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/display.h#newco... ui/gfx/display.h:162: int physical_width_mm_; On 2016/01/13 00:22:10, robliao wrote: > gfx::Size physical_size_mm; Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/screen_win.h File ui/gfx/screen_win.h (right): https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/screen_win.h#ne... ui/gfx/screen_win.h:8: #include <stdint.h> On 2016/01/13 00:22:10, robliao wrote: > Move this include to screen_win.cc Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:5: #include "ui/gfx/win/physical_size.h" On 2016/01/13 00:22:10, robliao wrote: > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes > > #include "ui/gfx/win/physical_size.h" > > #include <windows.h> (Exception to the rule since things tend to break if it > isn't first) > #include <setupapi.h> > > #include <iostream> > > #include "base/logging.h" > #include "base/memory/scoped_ptr.h" > #include "base/scoped_generic.h" > #include "base/strings/utf_string_conversions.h" > #include "base/win/registry.h" Done. The only problem seems to be missing a line break before <iostream>, if I missed something let me know. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:18: // https://msdn.microsoft.com/en-us/library/windows/hardware/ff545901.aspx On 2016/01/13 00:22:11, robliao wrote: > Nit: Add comment: > // {E6F07B5F-EE97-4A90-B076-33F57BF4EAA7} Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:35: device_info_list, device_info, DICS_FLAG_GLOBAL, 0, DIREG_DEV, KEY_READ)); On 2016/01/13 00:22:10, robliao wrote: > The preference in ui/gfx/* seems to be to line up the arguments when the > argument list exceeds the 80 char limit, falling back to this format when single > argument lined up this way exceeds the 80 char limit. > > base::win::RegKey reg_key(SetupDiOpenDevRegKey(device_info_list, > device_info, > DICS_FLAG_GLOBAL, > 0, > DIREG_DEV, > KEY_READ)); > > fits. Similar rules apply to assignments: > > type var_name = func_call(arg1, > arg2, > arg3) > (Only if arg1, arg2, arg3 would exceed the 80 char limit) > > Apply to the rest of the file. > > Personally, I prefer what you have right now, but we follow whatever is already > established in this section. I just ran git cl format for this... Should I be doing func(arg1, arg2, arg3, arg4) everywhere, or is it okay to do func(arg1, arg2, arg3, arg4) ? Should I ignore what git cl format does if it breaks that convention? https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:65: device_info->cbSize = sizeof(SP_DEVINFO_DATA); On 2016/01/13 00:22:10, robliao wrote: > Similar to Windows, the caller should be initializing this value. You can, > however, DCHECK(device_info->cbSize == sizeof(*device_info)) Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:71: if (success || GetLastError() != ERROR_INSUFFICIENT_BUFFER) { On 2016/01/13 00:22:10, robliao wrote: > As set up, this call is guaranteed to fail, so a check for success is really > optional here. However, feel free to keep the ERROR_INSUFFICIENT_BUFFER check. Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:91: // Get a handle to the list of device interfaces that are monitors via Setup. On 2016/01/13 00:22:11, robliao wrote: > Remove this comment. > See https://google.github.io/styleguide/cppguide.html#Comments under "Don'ts". > Apply to the rest of the file. I moved all the important information into a big header comment, since I'm a bit worried the flow of the function is hard to follow. Lemme know if it's overwrought. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:108: SP_DEVINFO_DATA device_info = {0}; On 2016/01/13 00:22:10, robliao wrote: > cbSize should be set up here as part of initialization. > device_info.cbSize = sizeof(device_info); > > Nit: {} Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:111: bool get_info_succeeded = On 2016/01/13 00:22:10, robliao wrote: > Optional: You could plausibly inline this into the "if" (same with "found" > below). Keeping the temp variables reads a bit better imo https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:119: DISPLAY_DEVICE display_device = {0}; On 2016/01/13 00:22:11, robliao wrote: > Nit: {} Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:120: display_device.cb = sizeof(DISPLAY_DEVICE); On 2016/01/13 00:22:10, robliao wrote: > Nit: sizeof(display_device) Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:127: DISPLAY_DEVICE attached_device = {0}; On 2016/01/13 00:22:10, robliao wrote: > Nit: {} Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:128: attached_device.cb = sizeof(DISPLAY_DEVICE); On 2016/01/13 00:22:11, robliao wrote: > Nit: sizeof(attached_device) Done. https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:136: // We've found the monitor that matches the Setup device. On 2016/01/13 00:22:10, robliao wrote: > Nit: Setup -> setup. Done.
https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:35: device_info_list, device_info, DICS_FLAG_GLOBAL, 0, DIREG_DEV, KEY_READ)); On 2016/01/13 18:02:48, Bret Sepulveda wrote: > On 2016/01/13 00:22:10, robliao wrote: > > The preference in ui/gfx/* seems to be to line up the arguments when the > > argument list exceeds the 80 char limit, falling back to this format when > single > > argument lined up this way exceeds the 80 char limit. > > > > base::win::RegKey reg_key(SetupDiOpenDevRegKey(device_info_list, > > device_info, > > DICS_FLAG_GLOBAL, > > 0, > > DIREG_DEV, > > KEY_READ)); > > > > fits. Similar rules apply to assignments: > > > > type var_name = func_call(arg1, > > arg2, > > arg3) > > (Only if arg1, arg2, arg3 would exceed the 80 char limit) > > > > Apply to the rest of the file. > > > > Personally, I prefer what you have right now, but we follow whatever is > already > > established in this section. > > I just ran git cl format for this... Should I be doing > func(arg1, > arg2, > arg3, > arg4) > everywhere, or is it okay to do > func(arg1, arg2, > arg3, arg4) > ? Should I ignore what git cl format does if it breaks that convention? Sticking to the output of clang-format/git cl format is fine. I prefer one-per-line in general, but machine-formattable is way better than any argument about it, so prefer not to deviate from git cl format unless it does something very crazy (in which case you should also file a bug against clang-format).
https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:35: device_info_list, device_info, DICS_FLAG_GLOBAL, 0, DIREG_DEV, KEY_READ)); On 2016/01/13 18:08:36, scottmg wrote: > On 2016/01/13 18:02:48, Bret Sepulveda wrote: > > On 2016/01/13 00:22:10, robliao wrote: > > > The preference in ui/gfx/* seems to be to line up the arguments when the > > > argument list exceeds the 80 char limit, falling back to this format when > > single > > > argument lined up this way exceeds the 80 char limit. > > > > > > base::win::RegKey reg_key(SetupDiOpenDevRegKey(device_info_list, > > > device_info, > > > DICS_FLAG_GLOBAL, > > > 0, > > > DIREG_DEV, > > > KEY_READ)); > > > > > > fits. Similar rules apply to assignments: > > > > > > type var_name = func_call(arg1, > > > arg2, > > > arg3) > > > (Only if arg1, arg2, arg3 would exceed the 80 char limit) > > > > > > Apply to the rest of the file. > > > > > > Personally, I prefer what you have right now, but we follow whatever is > > already > > > established in this section. > > > > I just ran git cl format for this... Should I be doing > > func(arg1, > > arg2, > > arg3, > > arg4) > > everywhere, or is it okay to do > > func(arg1, arg2, > > arg3, arg4) > > ? Should I ignore what git cl format does if it breaks that convention? > > Sticking to the output of clang-format/git cl format is fine. > > I prefer one-per-line in general, but machine-formattable is way better than any > argument about it, so prefer not to deviate from git cl format unless it does > something very crazy (in which case you should also file a bug against > clang-format). I'd defer to the owners on this one. I've gotten pushback in either way on various parts of the codebase. The general guidance from the style guide is to be consistent.
Non-Owner LGTM after the below comment is addressed https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/display.h File ui/gfx/display.h (right): https://codereview.chromium.org/1563183008/diff/100001/ui/gfx/display.h#newco... ui/gfx/display.h:143: void SetPhysicalSize(int width_mm, int height_mm); On 2016/01/13 18:02:48, Bret Sepulveda wrote: > On 2016/01/13 00:22:10, robliao wrote: > > SetPhysicalSizeMm(gfx::Size size_mm) for symmetry to GetPhysicalSizeMm. > > Done. This should still accept gfx::Size as an input.
bsep@chromium.org changed reviewers: + jbauman@chromium.org, sky@chromium.org
needs review by owners
https://codereview.chromium.org/1563183008/diff/120001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/120001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:104: interface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA); sizeof(interface_data)
https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/display.h File ui/gfx/display.h (right): https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/display.h#newco... ui/gfx/display.h:139: bool IsPhysicalSizeAvailable() const; Is there a compelling reason to add api here? I don't see this api used in this patch. https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.h (right): https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.h:16: std::string display_name; Structs have functions before members too.
https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:55: *width_mm = ((data[68] & 0xF0) << 4) + data[66]; Make sure you deal with error detection. For example, what if width_mm/height_mm is <= 0? Or what if you didn't read 69 bytes?
https://codereview.chromium.org/1563183008/diff/120001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/120001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:104: interface_data.cbSize = sizeof(SP_DEVICE_INTERFACE_DATA); On 2016/01/13 19:10:32, robliao wrote: > sizeof(interface_data) Done. https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/display.h File ui/gfx/display.h (right): https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/display.h#newco... ui/gfx/display.h:139: bool IsPhysicalSizeAvailable() const; On 2016/01/13 21:00:10, sky wrote: > Is there a compelling reason to add api here? I don't see this api used in this > patch. I talked with Scott and he's right, I'll revert this API change until someone actually needs it. https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:55: *width_mm = ((data[68] & 0xF0) << 4) + data[66]; On 2016/01/13 22:15:54, sky wrote: > Make sure you deal with error detection. For example, what if width_mm/height_mm > is <= 0? Or what if you didn't read 69 bytes? Done. https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.h (right): https://codereview.chromium.org/1563183008/diff/140001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.h:16: std::string display_name; On 2016/01/13 21:00:10, sky wrote: > Structs have functions before members too. Done.
https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/screen_win.cc File ui/gfx/screen_win.cc (right): https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/screen_win.cc#n... ui/gfx/screen_win.cc:30: int64_t GenerateDisplayId(const std::string& str) { Is this change still needed? https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:54: if (data[54] == 0) Might the read amount be < 54? https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:74: DCHECK(device_info->cbSize == sizeof(*device_info)); nit: DCHECK_EQ https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:77: SetupDiGetDeviceInterfaceDetail(device_info_list, interface_data, nullptr, 0, Don't you need to check the return value rather than GetLastError?
https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:77: SetupDiGetDeviceInterfaceDetail(device_info_list, interface_data, nullptr, 0, On 2016/01/14 15:36:58, sky wrote: > Don't you need to check the return value rather than GetLastError? In this case, SetupDiGetDeviceInterfaceDetail is guaranteed to fail and sets GetLastError with ERROR_INSUFFICIENT_BUFFER when provided with nullptr DeviceInterfaceDetailData and a DeviceInterfaceDetailDataSize of 0. A return check isn't needed, but a GetLastError check is needed to make sure that it took the buffer size codepath and didn't fail in some other way.
On 2016/01/14 18:07:06, robliao wrote: > https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... > File ui/gfx/win/physical_size.cc (right): > > https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... > ui/gfx/win/physical_size.cc:77: > SetupDiGetDeviceInterfaceDetail(device_info_list, interface_data, nullptr, 0, > On 2016/01/14 15:36:58, sky wrote: > > Don't you need to check the return value rather than GetLastError? > > In this case, SetupDiGetDeviceInterfaceDetail is guaranteed to fail and sets > GetLastError with ERROR_INSUFFICIENT_BUFFER when provided with nullptr > DeviceInterfaceDetailData and a DeviceInterfaceDetailDataSize of 0. A return > check isn't needed, but a GetLastError check is needed to make sure that it took > the buffer size codepath and didn't fail in some other way. Isn't that a round about way of checking if a function fails when it has a return value that indicates if it fails?
On 2016/01/14 20:39:05, sky wrote: > On 2016/01/14 18:07:06, robliao wrote: > > > https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... > > File ui/gfx/win/physical_size.cc (right): > > > > > https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... > > ui/gfx/win/physical_size.cc:77: > > SetupDiGetDeviceInterfaceDetail(device_info_list, interface_data, nullptr, 0, > > On 2016/01/14 15:36:58, sky wrote: > > > Don't you need to check the return value rather than GetLastError? > > > > In this case, SetupDiGetDeviceInterfaceDetail is guaranteed to fail and sets > > GetLastError with ERROR_INSUFFICIENT_BUFFER when provided with nullptr > > DeviceInterfaceDetailData and a DeviceInterfaceDetailDataSize of 0. A return > > check isn't needed, but a GetLastError check is needed to make sure that it > took > > the buffer size codepath and didn't fail in some other way. > > Isn't that a round about way of checking if a function fails when it has a > return value that indicates if it fails? Not really. In this case, the function will always return FALSE. As a result, it's not necessary to check the return result. What does matter is the way in which if failed, and that is obtained via GetLastError. We're only using this call to obtain the necessary size for the buffer and then we call the function again with the allocated buffer. Oddly enough, idiomatic Windows code often dispenses with the GetLastError check (correctly or incorrectly) and just checks to see that the size out param was set.
content/browser/gpu lgtm
On Thu, Jan 14, 2016 at 12:50 PM, <robliao@chromium.org> wrote: > On 2016/01/14 20:39:05, sky wrote: >> >> On 2016/01/14 18:07:06, robliao wrote: >> > > > > https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... >> >> > File ui/gfx/win/physical_size.cc (right): >> > >> > > > > https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... >> >> > ui/gfx/win/physical_size.cc:77: >> > SetupDiGetDeviceInterfaceDetail(device_info_list, interface_data, >> > nullptr, > > 0, >> >> > On 2016/01/14 15:36:58, sky wrote: >> > > Don't you need to check the return value rather than GetLastError? >> > >> > In this case, SetupDiGetDeviceInterfaceDetail is guaranteed to fail and >> > sets >> > GetLastError with ERROR_INSUFFICIENT_BUFFER when provided with nullptr >> > DeviceInterfaceDetailData and a DeviceInterfaceDetailDataSize of 0. A >> > return >> > check isn't needed, but a GetLastError check is needed to make sure that >> > it >> took >> > the buffer size codepath and didn't fail in some other way. > > >> Isn't that a round about way of checking if a function fails when it has a >> return value that indicates if it fails? > > > Not really. In this case, the function will always return FALSE. As a > result, > it's not necessary to check the return result. The docs don't mention it always fail. But either way, add a comment to this effect as others are likely to be confused as to why the code is the way it is. -Scott > What does matter is the way in which if failed, and that is obtained via > GetLastError. > > We're only using this call to obtain the necessary size for the buffer and > then > we call the function again with the allocated buffer. > Oddly enough, idiomatic Windows code often dispenses with the GetLastError > check > (correctly or incorrectly) and just checks to see > that the size out param was set. > > > https://codereview.chromium.org/1563183008/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/01/14 21:00:41, sky wrote: > On Thu, Jan 14, 2016 at 12:50 PM, <mailto:robliao@chromium.org> wrote: > > On 2016/01/14 20:39:05, sky wrote: > >> > >> On 2016/01/14 18:07:06, robliao wrote: > >> > > > > > > > > https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... > >> > >> > File ui/gfx/win/physical_size.cc (right): > >> > > >> > > > > > > > > https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... > >> > >> > ui/gfx/win/physical_size.cc:77: > >> > SetupDiGetDeviceInterfaceDetail(device_info_list, interface_data, > >> > nullptr, > > > > 0, > >> > >> > On 2016/01/14 15:36:58, sky wrote: > >> > > Don't you need to check the return value rather than GetLastError? > >> > > >> > In this case, SetupDiGetDeviceInterfaceDetail is guaranteed to fail and > >> > sets > >> > GetLastError with ERROR_INSUFFICIENT_BUFFER when provided with nullptr > >> > DeviceInterfaceDetailData and a DeviceInterfaceDetailDataSize of 0. A > >> > return > >> > check isn't needed, but a GetLastError check is needed to make sure that > >> > it > >> took > >> > the buffer size codepath and didn't fail in some other way. > > > > > >> Isn't that a round about way of checking if a function fails when it has a > >> return value that indicates if it fails? > > > > > > Not really. In this case, the function will always return FALSE. As a > > result, > > it's not necessary to check the return result. > > The docs don't mention it always fail. But either way, add a comment > to this effect as others are likely to be confused as to why the code > is the way it is. > > -Scott > > What does matter is the way in which if failed, and that is obtained via > > GetLastError. > > > > We're only using this call to obtain the necessary size for the buffer and > > then > > we call the function again with the allocated buffer. > > Oddly enough, idiomatic Windows code often dispenses with the GetLastError > > check > > (correctly or incorrectly) and just checks to see > > that the size out param was set. > > > > > > https://codereview.chromium.org/1563183008/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > It's in the remarks section. https://msdn.microsoft.com/en-us/library/windows/hardware/ff551120(v=vs.85).aspx Remarks Using this function to get details about an interface is typically a two-step process: 1. Get the required buffer size. Call SetupDiGetDeviceInterfaceDetail with a NULLDeviceInterfaceDetailData pointer, a DeviceInterfaceDetailDataSize of zero, and a valid RequiredSize variable. In response to such a call, this function returns the required buffer size at RequiredSize and fails with GetLastError returning ERROR_INSUFFICIENT_BUFFER.
https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/screen_win.cc File ui/gfx/screen_win.cc (right): https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/screen_win.cc#n... ui/gfx/screen_win.cc:30: int64_t GenerateDisplayId(const std::string& str) { On 2016/01/14 15:36:58, sky wrote: > Is this change still needed? I missed that. Reverted. https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:54: if (data[54] == 0) On 2016/01/14 15:36:58, sky wrote: > Might the read amount be < 54? I added a ZeroMemory call above, so if we read less than 54 bytes, byte 54 will still be 0. That's true for the width and height below too though. I added this check in case the EDID block has different data in place of the timing information (though it's supposed to be required). https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:74: DCHECK(device_info->cbSize == sizeof(*device_info)); On 2016/01/14 15:36:58, sky wrote: > nit: DCHECK_EQ Done. https://codereview.chromium.org/1563183008/diff/160001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:77: SetupDiGetDeviceInterfaceDetail(device_info_list, interface_data, nullptr, 0, On 2016/01/14 18:07:06, robliao wrote: > On 2016/01/14 15:36:58, sky wrote: > > Don't you need to check the return value rather than GetLastError? > > In this case, SetupDiGetDeviceInterfaceDetail is guaranteed to fail and sets > GetLastError with ERROR_INSUFFICIENT_BUFFER when provided with nullptr > DeviceInterfaceDetailData and a DeviceInterfaceDetailDataSize of 0. A return > check isn't needed, but a GetLastError check is needed to make sure that it took > the buffer size codepath and didn't fail in some other way. I added a comment like scott suggested to clarify this code.
LGTM
The CQ bit was checked by bsep@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org, robliao@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/1563183008/#ps200001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183008/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183008/200001
Message was sent while issue was closed.
Description was changed from ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors ========== to ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors ========== to ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors Committed: https://crrev.com/b840e2f4ec9132b931f7ad54fd8fd20285e4eaa3 Cr-Commit-Position: refs/heads/master@{#369635} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/b840e2f4ec9132b931f7ad54fd8fd20285e4eaa3 Cr-Commit-Position: refs/heads/master@{#369635}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1563183008/diff/200001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/200001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:21: 0xE6F07B5F, 0xEE97, 0x4A90, 0xB0, 0x76, 0x33, 0xF5, 0x7B, 0xF4, 0xEA, 0xA7}; clang/win says: ..\..\ui\gfx\win\physical_size.cc(21,33) : error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] 0xE6F07B5F, 0xEE97, 0x4A90, 0xB0, 0x76, 0x33, 0xF5, 0x7B, 0xF4, 0xEA, 0xA7}; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ { } I'll send you a CL. (see also https://code.google.com/p/chromium/codesearch#search/&q=const%5C%20guid.*=&sq...)
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/1593543002/ by vasilii@chromium.org. The reason for reverting is: Broke compilation on Win x64 GN (dbg) https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN%20%28dbg%29... physical_size.obj : error LNK2019: unresolved external symbol __imp_SetupDiDestroyDeviceInfoList referenced in function "public: static void __cdecl `anonymous namespace'::DeviceInfoListScopedTraits::Free(void *)" (?Free@DeviceInfoListScopedTraits@?A0xfcbad8c5@@SAXPEAX@Z) physical_size.obj : error LNK2019: unresolved external symbol __imp_SetupDiEnumDeviceInterfaces referenced in function "class std::vector<struct gfx::PhysicalDisplaySize,class std::allocator<struct gfx::PhysicalDisplaySize> > __cdecl gfx::GetPhysicalSizeForDisplays(void)" (?GetPhysicalSizeForDisplays@gfx@@YA?AV?$vector@UPhysicalDisplaySize@gfx@@V?$allocator@UPhysicalDisplaySize@gfx@@@std@@@std@@XZ) physical_size.obj : error LNK2019: unresolved external symbol __imp_SetupDiGetDeviceInterfaceDetailW referenced in function "bool __cdecl `anonymous namespace'::GetInterfaceDetailAndDeviceInfo(void *,struct _SP_DEVICE_INTERFACE_DATA *,class scoped_ptr<struct _SP_DEVICE_INTERFACE_DETAIL_DATA_W,struct base::FreeDeleter> *,struct _SP_DEVINFO_DATA *)" (?GetInterfaceDetailAndDeviceInfo@?A0xfcbad8c5@@YA_NPEAXPEAU_SP_DEVICE_INTERFACE_DATA@@PEAV?$scoped_ptr@U_SP_DEVICE_INTERFACE_DETAIL_DATA_W@@UFreeDeleter@base@@@@PEAU_SP_DEVINFO_DATA@@@Z) physical_size.obj : error LNK2019: unresolved external symbol __imp_SetupDiGetClassDevsW referenced in function "class std::vector<struct gfx::PhysicalDisplaySize,class std::allocator<struct gfx::PhysicalDisplaySize> > __cdecl gfx::GetPhysicalSizeForDisplays(void)" (?GetPhysicalSizeForDisplays@gfx@@YA?AV?$vector@UPhysicalDisplaySize@gfx@@V?$allocator@UPhysicalDisplaySize@gfx@@@std@@@std@@XZ) physical_size.obj : error LNK2019: unresolved external symbol __imp_SetupDiOpenDevRegKey referenced in function "bool __cdecl `anonymous namespace'::GetSizeFromRegistry(void *,struct _SP_DEVINFO_DATA *,int *,int *)" (?GetSizeFromRegistry@?A0xfcbad8c5@@YA_NPEAXPEAU_SP_DEVINFO_DATA@@PEAH2@Z) ./gfx.dll : fatal error LNK1120: 5 unresolved externals.
Message was sent while issue was closed.
huangs@chromium.org changed reviewers: + huangs@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1563183008/diff/200001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/200001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:85: reinterpret_cast<SP_DEVICE_INTERFACE_DETAIL_DATA*>(malloc(buffer_size))); This mixes malloc() and free, which is undefined behavior??
Message was sent while issue was closed.
https://codereview.chromium.org/1563183008/diff/200001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/200001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:85: reinterpret_cast<SP_DEVICE_INTERFACE_DETAIL_DATA*>(malloc(buffer_size))); I mean malloc() and delete.
Message was sent while issue was closed.
https://codereview.chromium.org/1563183008/diff/200001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/200001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:85: reinterpret_cast<SP_DEVICE_INTERFACE_DETAIL_DATA*>(malloc(buffer_size))); On 2016/01/15 15:41:05, huangs wrote: > I mean malloc() and delete. interface_detail has a Deleter of base::FreeDeleter.
Message was sent while issue was closed.
https://codereview.chromium.org/1563183008/diff/200001/ui/gfx/win/physical_si... File ui/gfx/win/physical_size.cc (right): https://codereview.chromium.org/1563183008/diff/200001/ui/gfx/win/physical_si... ui/gfx/win/physical_size.cc:85: reinterpret_cast<SP_DEVICE_INTERFACE_DETAIL_DATA*>(malloc(buffer_size))); Ah. So SP_DEVICE_INTERFACE_DETAIL_DATA needs to talk to C code, that's why malloc() is used? Would it be cleaner to wrap this into something like scoped_ptr<SP_DEVICE_INTERFACE_DETAIL_DATA, base::FreeDeleter> SP_DEVICE_INTERFACE_DETAIL_DATA::CreateInstance() {} ?
Message was sent while issue was closed.
Description was changed from ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors Committed: https://crrev.com/b840e2f4ec9132b931f7ad54fd8fd20285e4eaa3 Cr-Commit-Position: refs/heads/master@{#369635} ========== to ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors Committed: https://crrev.com/b840e2f4ec9132b931f7ad54fd8fd20285e4eaa3 Cr-Commit-Position: refs/heads/master@{#369635} ==========
The CQ bit was checked by bsep@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, scottmg@chromium.org, jbauman@chromium.org, robliao@chromium.org Link to the patchset: https://codereview.chromium.org/1563183008/#ps220001 (title: "fix gn build and applied guid brace warning fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183008/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183008/220001
Message was sent while issue was closed.
Description was changed from ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors Committed: https://crrev.com/b840e2f4ec9132b931f7ad54fd8fd20285e4eaa3 Cr-Commit-Position: refs/heads/master@{#369635} ========== to ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors Committed: https://crrev.com/b840e2f4ec9132b931f7ad54fd8fd20285e4eaa3 Cr-Commit-Position: refs/heads/master@{#369635} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors Committed: https://crrev.com/b840e2f4ec9132b931f7ad54fd8fd20285e4eaa3 Cr-Commit-Position: refs/heads/master@{#369635} ========== to ========== Added capability on Windows to get the physical dimensions of your attached monitors. Also added this information to about:gpu for testing. BUG=547914 TEST=go to the about:gpu URL and check for "Diagonal Monitor Size". it should show the size of your monitors Committed: https://crrev.com/b840e2f4ec9132b931f7ad54fd8fd20285e4eaa3 Cr-Commit-Position: refs/heads/master@{#369635} Committed: https://crrev.com/b2f706f91654deb8ffea59a2be4bdc38c1f2bb7f Cr-Commit-Position: refs/heads/master@{#370180} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/b2f706f91654deb8ffea59a2be4bdc38c1f2bb7f Cr-Commit-Position: refs/heads/master@{#370180} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
