Grafxbot has code to get this information so it should be pretty straightforward to do once GfxInfo lands. http://bitbucket.org/jonallengriffin/grafxbot/

New repo is: http://hg.mozilla.org/automation/grafxbot

Jonathan, how accurate is the reporting of VRAM with grafxbot?

> Jonathan, how accurate is the reporting of VRAM with grafxbot? It seems accurate, and I haven't had any reports of problems, but there's no guarantee. I was unable to find a canonical way to get this information on Windows without using certain DirectX calls, which may not be available on every system (although I suppose they would be available on the systems we most care about...e.g., Win 7).

(In reply to comment #3) > > Jonathan, how accurate is the reporting of VRAM with grafxbot? > > It seems accurate, and I haven't had any reports of problems, but there's no > guarantee. I was unable to find a canonical way to get this information on > Windows without using certain DirectX calls, which may not be available on > every system (although I suppose they would be available on the systems we most > care about...e.g., Win 7). What does it report on systems with integrated graphics hardware? What are the DirectX calls for finding out this info?

> What does it report on systems with integrated graphics hardware? For integrated devices, it seems to report the max amount of shared RAM it can allocate, e.g., Intel(R) (0x8086) Intel(R) 4 Series Express Chipset Family 0x2A42 [+] 1340MB Intel(R) (0x8086) G45/G43 Express Chipset 0x2E22 [+] 782MB > What are the DirectX calls for finding out this info? I don't recall, it's been a while since I looked into this, I'll dig around again.

>> What are the DirectX calls for finding out this info? > I don't recall, it's been a while since I looked into this, I'll dig around > again. There are several ways according to Microsoft: http://msdn.microsoft.com/en-us/library/ee419018%28v=VS.85%29.aspx There doesn't seem to be any one-size-fits-all approach.

I'm not completely sure what I'm doing here so review harshly.

This version gets rid of the display size stuff. We can add that back if we need it.

This patch is a bit tricky in the sense that the ID3D10Device1 is not 100% guaranteed to be created off this device. It's very likely to, though.

(In reply to comment #9) > This patch is a bit tricky in the sense that the ID3D10Device1 is not 100% > guaranteed to be created off this device. It's very likely to, though. True, we should probably do some investigation what to do about that. Bas, do you know how windows handles multi-gpu laptops?

I have never seen or touched a laptop with two GPUs. I know how two seperate desktop GPUs work though. You get more entries in your registry for the different devices. And you can pick which one is your main desktop, which one is extended, etc.

Jeff, can I (pretty strongly!) suggest that GfxInfo implement nsIPropertyBag, and that all the gfx details are made available as propertybag members? That way we can easily add new entries without having to rev the interface at all. about:support can localize the keys it knows about, and just display the rest "raw", so that we don't have to worry about localization down the line if we need to collect extra data quickly.

(In reply to comment #12) > Jeff, can I (pretty strongly!) suggest that GfxInfo implement nsIPropertyBag, > and that all the gfx details are made available as propertybag members? That > way we can easily add new entries without having to rev the interface at all. > about:support can localize the keys it knows about, and just display the rest > "raw", so that we don't have to worry about localization down the line if we > need to collect extra data quickly. Yes. That sounds lovely. I was hoping we could have something like that.

(In reply to comment #13) > (In reply to comment #12) > > Jeff, can I (pretty strongly!) suggest that GfxInfo implement nsIPropertyBag, > > and that all the gfx details are made available as propertybag members? That > > way we can easily add new entries without having to rev the interface at all. > > about:support can localize the keys it knows about, and just display the rest > > "raw", so that we don't have to worry about localization down the line if we > > need to collect extra data quickly. > > Yes. That sounds lovely. I was hoping we could have something like that. I tried out nsIPropertyBag and didn't really like the interface that it exposed. Long term I'd like to move to something like that, but for now I'd like to get something in as soon as possible.

Just putting this up here for reference.

Comment on attachment 466730 [details] [diff] [review] Second try >diff --git a/toolkit/locales/en-US/chrome/global/aboutSupport.properties b/toolkit/locales/en-US/chrome/global/aboutSupport.properties >+notifyRightsText = %S is free and open source software from the non-profit Mozilla Foundation. copy/pasto >diff --git a/widget/public/nsIGfxInfo.idl b/widget/public/nsIGfxInfo.idl > interface nsIGfxInfo : nsISupports capitalized first-letter getters are kind of weird, but I don't really care. >diff --git a/widget/src/windows/GfxInfo.cpp b/widget/src/windows/GfxInfo.cpp >+static int GetKeyValue(const TCHAR* keyLocation, const TCHAR* keyName, nsAString* destString, int type) Looks like this wants to return nsresult, though given the codes returned may as well just return bool (success). >+static void getDriverDetails(nsString& driverId, nsString& aDriverVersion, nsString& aDriverDate) I think this should just be in Init() rather than separate. >+ nsString wantedDriverId(driverId); use nsAutoString for local variables (though this will go away once getDriverDetails is moved into Init()) >+NS_IMETHODIMP GfxInfo::GetDisplayAdapter(nsAString & aDisplayAdapter) >+NS_IMETHODIMP GfxInfo::GetDisplayDriverVersion(nsAString & aDisplayDriverVersion) >+NS_IMETHODIMP GfxInfo::GetDisplayDriverDate(nsAString & aDisplayDriverDate) These can use assignment syntax (aFoo = mFoo). >+NS_IMETHODIMP GfxInfo::GetDisplayChipset(nsAString & aDisplayChipset) >+NS_IMETHODIMP GfxInfo::GetDisplayVendorID(nsAString & aDisplayVendorID) String munging in these is kind of gross, but not sure there's a better alternative. >+NS_IMETHODIMP GfxInfo::GetDisplayRAM(nsAString & aDisplayRAM) >+ return GetKeyValue(mDeviceKey.BeginReading() + 18, L"HardwareInformation.MemorySize", &aDisplayRAM, REG_DWORD); mDeviceKey is only ever used as |mDeviceKey.BeginReading() + 18|, so might as well just make it a PRUnichar* or whatever. >diff --git a/widget/src/windows/GfxInfo.h b/widget/src/windows/GfxInfo.h >+ nsresult Init(); Should just be void.

This should be cleaner and more robust than the previous approach

Same features as last time but actually with them.

can the data being gathered here be added to the crash reporting we do in the breakpad client?

Comment on attachment 467553 [details] [diff] [review] v4 >diff --git a/toolkit/content/aboutSupport.js b/toolkit/content/aboutSupport.js >+ // pad with zeros. (printf would be nicer) >+ createElement("td", String('0000'+gfxInfo.adapterVendorID.toString(16)).slice(-4)), >+ createElement("td", String('0000'+gfxInfo.adapterDeviceID.toString(16)).slice(-4)), Are these guaranteed to fit in 16 bits? >diff --git a/toolkit/locales/en-US/chrome/global/aboutSupport.properties b/toolkit/locales/en-US/chrome/global/aboutSupport.properties >+direct2DEnabled = Direct2D Enabled >+directWriteEnabled = DirectWrite Enabled >+adapterDescription = Adapter Description >+adapterVendorID = Vendor ID >+adapterDeviceID = Device ID >+adapterDrivers = Adapter Drivers >+adapterRAM = Adapter RAM >+driverVersion = Driver Version >+driverDate = Driver Date Some LOCALIZATION NOTES that explain what a localizer should do with these wouldn't hurt (e.g. "don't translate 'Direct2d'", "feel free to leave english strings if there are no good translations", "these are only used in about:support", etc.) (see https://developer.mozilla.org/en/xul_coding_style_guidelines#Localization_Notes ) >diff --git a/widget/src/windows/GfxInfo.cpp b/widget/src/windows/GfxInfo.cpp >+/* XXX: this doesn't handle multiple GPUs. We should try to do that */ file a followup and cite bug # in TODO? >+static nsresult GetKeyValue(const TCHAR* keyLocation, const TCHAR* keyName, nsAString& destString, int type) This method seems to make assumptions about the data returned from RegQueryValueEx that I'm not sure are safe. >+ switch (type) { >+ case REG_BINARY: { >+ // This is actually a wide string, that we must convert to a multi-byte string >+ dwcbData = sizeof(wCharValue); >+ result = RegQueryValueEx(key, keyName, 0, NULL, (LPBYTE)wCharValue, &dwcbData); s/0, NULL/NULL, NULL/ (other calls too) >+void GfxInfo::Init() >+ result = RegQueryValueExW(subkey, L"DriverVersion", 0, NULL, (LPBYTE)value, &dwcbData); stray tab The registry walking code here makes me a bit nervous too, again because I'm not familiar with the API. >+NS_IMETHODIMP GfxInfo::GetAdapterRAM(nsAString & aAdapterRAM) >+{ >+ if (NS_SUCCEEDED(GetKeyValue(mDeviceKey.BeginReading(), L"HardwareInformation.MemorySize", aAdapterRAM, REG_DWORD))) >+ aAdapterRAM = "Unknown"; if (NS_FAILED()), and also please make this compile (GfxInfo::GetAdapterDriver too) Should the other string properties have default values too? I think date/version should be obtained in the getter rather than on construction, but it sounded like you wanted to avoid doing that. >+NS_IMETHODIMP GfxInfo::GetAdapterVendorID(PRUint32 *aAdapterVendorID) >+NS_IMETHODIMP GfxInfo::GetAdapterDeviceID(PRUint32 *aAdapterDeviceID) I'd like it if these shared code. >+ PRInt32 err; >+ *aAdapterVendorID = vendor.ToInteger(&err, 16); nsresult err; Having someone more familiar with the registry APIs check this patch couldn't hurt.

With your move to a .properties file here, you need to remove the entry you put in the .dtd file.

(In reply to comment #19) > can the data being gathered here be added to the crash reporting we do in the > breakpad client? that's bug 586048.

Also, you need to change your strings "Unknown" to L"Unknown"; otherwise, you won't compile, at least on my machine (MSVC2010).

Final comments addressed.

There seems to be some garbage at the end of "Adapter drivers" with v5: http://grab.by/66j0

Try to fix up the build problems happening with thunderbird and do a better job parsing multistring registry keys.

Comment on attachment 470024 [details] [diff] [review] Better fixups >@@ -97,17 +95,34 @@ static nsresult GetKeyValue(const TCHAR* > } > case REG_MULTI_SZ: { > // A chain of null-separated strings; we convert the nulls to spaces >- dwcbData = sizeof(tCharValue); >- result = RegQueryValueExW(key, keyName, NULL, NULL, (LPBYTE)tCharValue, &dwcbData); >+ WCHAR wCharValue[1024]; >+ dwcbData = sizeof(wCharValue); Rename dwcbData to something that is actually meaningful? >+ result = RegQueryValueExW(key, keyName, NULL, NULL, (LPBYTE)wCharValue, &dwcbData); You should pass a DWORD as the fourth parameter to this function and check to make sure that it's REG_MULTI_SZ. > if (result != ERROR_SUCCESS) { > retval = NS_ERROR_FAILURE; > } You should not proceed if result is not ERROR_SUCCESS. Otherwise you'll probably be readin random bytes off the stack. > // This bit here could probably be cleaner. >- for (DWORD i = 0, len = dwcbData/sizeof(tCharValue[0]); i < len; i++) { >- if (tCharValue[i] == '\0') >- tCharValue[i] = ' '; >+ WCHAR *begin = &wCharValue[0]; >+ DWORD len = dwcbData/sizeof(wCharValue[0]); >+ for (DWORD i = 0; i < len; i++) { >+ if (wCharValue[i] == '\0') { >+ if (&wCharValue[i] == begin) { >+ // We've found an empty string; we're done. >+ break; >+ } else { >+ wCharValue[i] = ' '; >+ // set begin to the begining of the next string >+ begin = &wCharValue[i+1]; >+ } >+ } > } Here is how I would write this loop: PRBool isValid = PR_FALSE; for (DWORD i = 0; i < len; ++i) { if (wCharValue[i] == '\0') { wCharValue[i] = ' '; if (i < len - 1 && wCharValue[i + 1] == '\0') { isValid = PR_TRUE; break; } } } Check |isValid| after the loop and discard the value if it's false. >- destString = tCharValue; >+ >+ // ensure wCharValue is null terminated >+ wCharValue[len-1] = '\0'; This is probably not needed, but maybe you can replace it with an assertion? >- while (EnumDisplayDevices(NULL, deviceIndex, &lpDisplayDevice, 0)) { >- if (lpDisplayDevice.StateFlags & DISPLAY_DEVICE_PRIMARY_DEVICE) >+ while (EnumDisplayDevicesW(NULL, deviceIndex, &displayDevice, 0)) { >+ if (displayDevice.StateFlags & DISPLAY_DEVICE_PRIMARY_DEVICE) > break; > deviceIndex++; > } You're not checking deviceIndex. In case the first call to EnumDisplayDevicesW fails, displayDevice might be filled with random data. > /* DeviceKey is "reserved" according to MSDN so we'll be careful with it */ >- if (wcsncmp(lpDisplayDevice.DeviceKey, DEVICE_KEY_PREFIX, wcslen(DEVICE_KEY_PREFIX)) != 0) >+ if (wcsncmp(displayDevice.DeviceKey, DEVICE_KEY_PREFIX, NS_ARRAY_LENGTH(DEVICE_KEY_PREFIX)-1) != 0) > return; Is this correct? It seems to me that this check makes sure that DeviceKey *begins* with DEVICE_KEY_PREFIX. I think you should remove the |-1| to make sure that the terminating null character is compared as well. > // make sure the string is NULL terminated > size_t i; >- for (i = 0; i < sizeof(lpDisplayDevice.DeviceKey); i++) { >- if (lpDisplayDevice.DeviceKey[i] == L'\0') >+ for (i = 0; i < NS_ARRAY_LENGTH(displayDevice.DeviceKey); i++) { >+ if (displayDevice.DeviceKey[i] == L'\0') > break; > } > >- if (i == sizeof(lpDisplayDevice.DeviceKey)) { >+ if (i == NS_ARRAY_LENGTH(displayDevice.DeviceKey)) { > // we did not find a NULL > return; > } Suggestion: use wcsnlen instead.

Comment on attachment 470046 [details] [diff] [review] Betterer fixups r=me on a version of this patch which assumes that DWORDs are unsigned integers, checks |isValid|, and does not assign PR_TRUE to a bool. :-)

(In reply to comment #30) > Comment on attachment 470046 [details] [diff] [review] > Betterer fixups > > r=me on a version of this patch which assumes that DWORDs are unsigned > integers, checks |isValid|, and does not assign PR_TRUE to a bool. :-) I as wrong on the first comment!

wrong font is used/displayed. with new/clean profile. Windows 7 pro Japaneses.

(In reply to comment #33) sorry, fine with latest Hourly. http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1282958805/

Two SeaMonkey tinderboxes on fire: http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1282978356.1282978738.2729.gz http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1282978356.1282978685.2438.gz e:/builds/slave/comm-central-trunk-win32-debug/build/mozilla/widget/src/windows/GfxInfo.cpp(173) : error C2679: binary '=' : no operator found which takes a right-hand operand of type 'CHAR [128]' (or there is no acceptable conversion) e:\builds\slave\comm-central-trunk-win32-debug\build\objdir\mozilla\dist\include\nsTString.h(104): could be 'nsString::self_type &nsString::operator =(nsAString_internal::char_type)' e:\builds\slave\comm-central-trunk-win32-debug\build\objdir\mozilla\dist\include\nsTString.h(105): or 'nsString::self_type &nsString::operator =(const nsAString_internal::char_type *)' e:\builds\slave\comm-central-trunk-win32-debug\build\objdir\mozilla\dist\include\nsTString.h(106): or 'nsString::self_type &nsString::operator =(const nsString::self_type &)' e:\builds\slave\comm-central-trunk-win32-debug\build\objdir\mozilla\dist\include\nsTString.h(107): or 'nsString::self_type &nsString::operator =(const nsAString_internal::substring_type &)' e:\builds\slave\comm-central-trunk-win32-debug\build\objdir\mozilla\dist\include\nsTString.h(108): or 'nsString::self_type &nsString::operator =(const nsAString_internal::substring_tuple_type &)' while trying to match the argument list '(nsString, CHAR [128])' e:/builds/slave/comm-central-trunk-win32-debug/build/mozilla/widget/src/windows/GfxInfo.cpp(194) : error C2664: 'RegEnumKeyExW' : cannot convert parameter 3 from 'TCHAR [64]' to 'LPWSTR' Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast NEXT ERROR e:/builds/slave/comm-central-trunk-win32-debug/build/mozilla/widget/src/windows/GfxInfo.cpp(194) : fatal error C1903: unable to recover from previous error(s); stopping compilation

perhaps something like this: http://mxr.mozilla.org/mozilla-central/source/browser/components/shell/src/nsWindowsShellService.cpp#1004 #ifndef MAX_BUF #define MAX_BUF 4096 #endif .... PRUnichar subkeyName[MAX_BUF]; DWORD len = sizeof subkeyName; res = ::RegEnumKeyExW(mailKey, i++, subkeyName, &len, NULL, NULL, NULL, NULL); if (REG_SUCCEEDED(res)) { HKEY accountKey; res = ::RegOpenKeyExW(mailKey, PromiseFlatString(subkeyName).get(), 0, KEY_READ, &accountKey); if (REG_SUCCEEDED(res)) { *aResult = accountKey; // Close the key we opened. ::RegCloseKey(mailKey); return PR_TRUE; }

(In reply to comment #35) > Two SeaMonkey tinderboxes on fire: And TB too: I filed bug 591576, fwiw.