Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23707010:
Implement Display id field in ScreenWin. (Closed)
|
Created:
7 years, 3 months ago by Haojian Wu Modified:
7 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement Display id field in ScreenWin.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221237
Patch Set 1 #
Total comments: 5
Patch Set 2 : Update #Messages
Total messages: 8 (0 generated)
Please review it. Thanks.
https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc File ui/gfx/screen_win.cc (left): https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc#oldcode17 ui/gfx/screen_win.cc:17: MONITORINFO monitor_info = { 0 }; Any reason to prefer ZeroMemory over standard initialization? https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc File ui/gfx/screen_win.cc (right): https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc#newcode35 ui/gfx/screen_win.cc:35: int64 id = static_cast<int64>(base::Hash(WideToUTF8(device.DeviceID))); Hmm, according to http://msdn.microsoft.com/en-us/library/windows/desktop/dd183569(v=vs.85).aspx DeviceID is marked as "Not Used", although one comment says it does have some value. I'm not familiar enough on windows to say this is safe to use or not. Do you think szDevice is safer to use? Also please get the id first: int64 id = 0; if (..) id = gfx::Display(id, bounds)
https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc File ui/gfx/screen_win.cc (left): https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc#oldcode17 ui/gfx/screen_win.cc:17: MONITORINFO monitor_info = { 0 }; On 2013/08/30 18:15:13, oshima wrote: > Any reason to prefer ZeroMemory over standard initialization? According to http://en.cppreference.com/w/cpp/language/aggregate_initialization Since MONITORINFOEX is a subclass of MONITORINFO, we can't use aggregate initialization here. https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc File ui/gfx/screen_win.cc (right): https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc#newcode35 ui/gfx/screen_win.cc:35: int64 id = static_cast<int64>(base::Hash(WideToUTF8(device.DeviceID))); On 2013/08/30 18:15:13, oshima wrote: > Hmm, according to > http://msdn.microsoft.com/en-us/library/windows/desktop/dd183569%28v=vs.85%29... > > DeviceID is marked as "Not Used", although one comment says > it does have some value. I'm not familiar enough on windows to > say this is safe to use or not. Do you think szDevice is safer to use? > > Also please get the id first: > > int64 id = 0; > if (..) > id = > > gfx::Display(id, bounds) Done.
https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc File ui/gfx/screen_win.cc (left): https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc#oldcode17 ui/gfx/screen_win.cc:17: MONITORINFO monitor_info = { 0 }; On 2013/08/31 14:17:07, Haojian Wu wrote: > On 2013/08/30 18:15:13, oshima wrote: > > Any reason to prefer ZeroMemory over standard initialization? > > According to http://en.cppreference.com/w/cpp/language/aggregate_initialization > > Since MONITORINFOEX is a subclass of MONITORINFO, we can't use aggregate > initialization here. but according to http://msdn.microsoft.com/en-us/library/windows/desktop/dd145066(v=vs.85).aspx, MONITORINFOEX is plain struct, not subclass. (I believe this still has to work with C, not C++?)
On 2013/09/03 18:04:44, oshima wrote: > https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc > File ui/gfx/screen_win.cc (left): > > https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc#oldcode17 > ui/gfx/screen_win.cc:17: MONITORINFO monitor_info = { 0 }; > On 2013/08/31 14:17:07, Haojian Wu wrote: > > On 2013/08/30 18:15:13, oshima wrote: > > > Any reason to prefer ZeroMemory over standard initialization? > > > > According to > http://en.cppreference.com/w/cpp/language/aggregate_initialization > > > > Since MONITORINFOEX is a subclass of MONITORINFO, we can't use aggregate > > initialization here. > > but according to > http://msdn.microsoft.com/en-us/library/windows/desktop/dd145066%28v=vs.85%29..., > MONITORINFOEX is plain struct, > not subclass. (I believe this still has to work with C, not C++?) Yes, it works with C. If you look into the head file: WinUser.h, you will find it declare differently between C and C++, see the following declaring statements below: typedef struct tagMONITORINFO { DWORD cbSize; RECT rcMonitor; RECT rcWork; DWORD dwFlags; } MONITORINFO, *LPMONITORINFO; #ifdef __cplusplus typedef struct tagMONITORINFOEXA : public tagMONITORINFO { CHAR szDevice[CCHDEVICENAME]; } MONITORINFOEXA, *LPMONITORINFOEXA; typedef struct tagMONITORINFOEXW : public tagMONITORINFO { WCHAR szDevice[CCHDEVICENAME]; } MONITORINFOEXW, *LPMONITORINFOEXW; #ifdef UNICODE typedef MONITORINFOEXW MONITORINFOEX; typedef LPMONITORINFOEXW LPMONITORINFOEX; #else typedef MONITORINFOEXA MONITORINFOEX; typedef LPMONITORINFOEXA LPMONITORINFOEX; #endif // UNICODE #else // ndef __cplusplus typedef struct tagMONITORINFOEXA { MONITORINFO; CHAR szDevice[CCHDEVICENAME]; } MONITORINFOEXA, *LPMONITORINFOEXA; typedef struct tagMONITORINFOEXW { MONITORINFO; WCHAR szDevice[CCHDEVICENAME]; } MONITORINFOEXW, *LPMONITORINFOEXW; #ifdef UNICODE typedef MONITORINFOEXW MONITORINFOEX; typedef LPMONITORINFOEXW LPMONITORINFOEX; #else typedef MONITORINFOEXA MONITORINFOEX; typedef LPMONITORINFOEXA LPMONITORINFOEX; #endif // UNICODE
On 2013/09/04 00:18:50, Haojian Wu wrote: > On 2013/09/03 18:04:44, oshima wrote: > > https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc > > File ui/gfx/screen_win.cc (left): > > > > https://codereview.chromium.org/23707010/diff/1/ui/gfx/screen_win.cc#oldcode17 > > ui/gfx/screen_win.cc:17: MONITORINFO monitor_info = { 0 }; > > On 2013/08/31 14:17:07, Haojian Wu wrote: > > > On 2013/08/30 18:15:13, oshima wrote: > > > > Any reason to prefer ZeroMemory over standard initialization? > > > > > > According to > > http://en.cppreference.com/w/cpp/language/aggregate_initialization > > > > > > Since MONITORINFOEX is a subclass of MONITORINFO, we can't use aggregate > > > initialization here. > > > > but according to > > > http://msdn.microsoft.com/en-us/library/windows/desktop/dd145066%2528v=vs.85%..., > > MONITORINFOEX is plain struct, > > not subclass. (I believe this still has to work with C, not C++?) > > Yes, it works with C. > If you look into the head file: WinUser.h, you will find it declare differently > between C and C++, see the following declaring statements below: That is ... awful :( Oh well, lgtm. > > typedef struct tagMONITORINFO > { > DWORD cbSize; > RECT rcMonitor; > RECT rcWork; > DWORD dwFlags; > } MONITORINFO, *LPMONITORINFO; > > #ifdef __cplusplus > typedef struct tagMONITORINFOEXA : public tagMONITORINFO > { > CHAR szDevice[CCHDEVICENAME]; > } MONITORINFOEXA, *LPMONITORINFOEXA; > typedef struct tagMONITORINFOEXW : public tagMONITORINFO > { > WCHAR szDevice[CCHDEVICENAME]; > } MONITORINFOEXW, *LPMONITORINFOEXW; > #ifdef UNICODE > typedef MONITORINFOEXW MONITORINFOEX; > typedef LPMONITORINFOEXW LPMONITORINFOEX; > #else > typedef MONITORINFOEXA MONITORINFOEX; > typedef LPMONITORINFOEXA LPMONITORINFOEX; > #endif // UNICODE > #else // ndef __cplusplus > typedef struct tagMONITORINFOEXA > { > MONITORINFO; > CHAR szDevice[CCHDEVICENAME]; > } MONITORINFOEXA, *LPMONITORINFOEXA; > typedef struct tagMONITORINFOEXW > { > MONITORINFO; > WCHAR szDevice[CCHDEVICENAME]; > } MONITORINFOEXW, *LPMONITORINFOEXW; > #ifdef UNICODE > typedef MONITORINFOEXW MONITORINFOEX; > typedef LPMONITORINFOEXW LPMONITORINFOEX; > #else > typedef MONITORINFOEXA MONITORINFOEX; > typedef LPMONITORINFOEXA LPMONITORINFOEX; > #endif // UNICODE
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/23707010/4001
Message was sent while issue was closed.
Change committed as 221237 |
