704710 - Refactor GfxDriverInfo/GfxInfo(Base) to support string device and vendor IDs

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Doug Sherk (:drs) (inactive)]

Android specifies everything in terms of a string-based device or vendor ID. This makes it difficult to properly compare the hardware data to a blocklist, so we need to change the GfxDriverInfo/GfxInfo(Base) hardware handling classes to support string-based device and vendor IDs.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0, port blocklist vendor/device IDs to strings.

This patch changes the GfxDriverInfo and GfxInfo(Base) classes to use strings for their device and vendor IDs. It also includes a fix for bug 699482, which was backed out due to a reftest failure. Note that this patch depends on the one in bug 699482, so they should be landed together.

Comment 2

[Doug Sherk (:drs) (inactive)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=4aaf8da92862

Second try push to test whether or not it fixed the reftest failure for bug 699482: https://tbpl.mozilla.org/?tree=Try&rev=8ca6453ee424

Comment 3

[Doug Sherk (:drs) (inactive)]

Nevermind about the secondary fix. I actually included it in a change that was made to the patch in bug 699482. Will post an update to that patch in that bug thread.

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 576383 [details] [diff] [review]
Patch v1.0, port blocklist vendor/device IDs to strings.

Review of attachment 576383 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/android/GfxInfo.cpp
@@ +183,5 @@
>  {
>    return NS_ERROR_FAILURE;
>  }
>  
>  /* readonly attribute unsigned long adapterVendorID; */

You'll want to change (or remove) all these comments.

@@ +252,2 @@
>    CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterDeviceID"),
> +      narrowDeviceID);

While you're modifying these lines, can you indent the second argument to AnnotateCrashReport properly here?

::: widget/src/windows/GfxInfo.cpp
@@ +425,5 @@
>          setupDestroyDeviceInfoList(devinfo);
>        }
>  
> +      CopyASCIItoUTF16(nsPrintfCString("0x%04x", ParseIDFromDeviceID(mDeviceID,  "VEN_", 4)), mAdapterVendorID);
> +      CopyASCIItoUTF16(nsPrintfCString("0x%04x", ParseIDFromDeviceID(mDeviceID,  "&DEV_", 4)), mAdapterDeviceID);

Can you use mAdapterVendorID.AppendPrintf() here?

@@ +481,2 @@
>                  adapterVendorID2 = ParseIDFromDeviceID(deviceID2, "VEN_", 4);
> +                CopyASCIItoUTF16(nsPrintfCString("0x%04x", adapterVendorID2), adapterVendorID2String);

Similar question here.

@@ +519,5 @@
>                    mDeviceKey2 = driverKey2;
>                    mDriverVersion2 = driverVersion2;
>                    mDriverDate2 = driverDate2;
> +                  CopyASCIItoUTF16(nsPrintfCString("0x%04x", adapterVendorID2), mAdapterVendorID2);
> +                  CopyASCIItoUTF16(nsPrintfCString("0x%04x", adapterDeviceID2), mAdapterDeviceID2);

And here.

@@ +544,5 @@
>    }
>  
>    const char *spoofedVendor = PR_GetEnv("MOZ_GFX_SPOOF_VENDOR_ID");
>    if (spoofedVendor) {
> +     CopyASCIItoUTF16(spoofedVendor, mAdapterVendorID);

And here.

@@ +572,5 @@
>    }
>  
>    const char *spoofedDevice = PR_GetEnv("MOZ_GFX_SPOOF_DEVICE_ID");
>    if (spoofedDevice) {
> +    CopyASCIItoUTF16(spoofedDevice, mAdapterDeviceID);

etc

@@ +756,2 @@
>    CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("AdapterDeviceID"),
> +      narrowDeviceID);

Can you fix the indentation here?

::: widget/src/xpwidgets/GfxDriverInfo.cpp
@@ +88,5 @@
>      mDriverVersion(aOrig.mDriverVersion),
>      mDriverVersionMax(aOrig.mDriverVersionMax),
>      mSuggestedVersion(aOrig.mSuggestedVersion)
>  {
>    // If we're managing the lifetime of the devices array, we have to make a

s/devices array/device family/

@@ +96,4 @@
>  
> +    mDevices->SetLength(aOrig.mDevices->Length());
> +    for (PRUint32 i = 0; i < aOrig.mDevices->Length(); i++)
> +      (*mDevices)[i] = (*aOrig.mDevices)[i];

Does nsTArray<nsString>'s operator= not handle this for us?

::: widget/src/xpwidgets/GfxDriverInfo.h
@@ +52,5 @@
> +// Macros for getting a vendor id quickly.
> +#define GET_VENDOR_ID(name) ((nsAString&)(GfxDriverInfo::GetDeviceVendor(name)))
> +
> +// Macros for getting a device family quickly.
> +#define GET_DEVICE_FAMILY(devFamily) ((GfxDeviceFamily*)GfxDriverInfo::GetDeviceFamily(devFamily))

I am not super-enthusiastic about putting these two macros in the global namespace, but I am especially not enthusiastic about them casting the return values of the functions. It'd be better if the functions didn't return const, since we seem to not want to use the constness.

::: widget/src/xpwidgets/GfxInfoBase.cpp
@@ +91,5 @@
> +        delete GfxDriverInfo::mDeviceFamilies[i];
> +
> +    for (PRUint32 i = 0; i < DeviceVendorMax; i++)
> +      if (GfxDriverInfo::mDeviceVendors[i])
> +        delete GfxDriverInfo::mDeviceVendors[i];

You don't need if (ptr) delete ptr; delete handles that if for you.

@@ +323,5 @@
>    PRUint32 length;
>    if (NS_FAILED(aDevices->GetLength(&length)))
>      return nsnull;
>  
>    // For each <device>, get its device ID, and return a freshly-allocated array

s/array/GfxDeviceFamily/

@@ +558,3 @@
>        }
> +      // Prevent di falling out of scope from destroying the devices.
> +      di.mDeleteDevices = false;

ugh. How was this not a problem before?

Comment 5

[Joe Drew (not getting mail)]

Oh, one additional question: you have nsString.Equals() all over the place - does operator== not work?

Comment 6

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1, port blocklist vendor/device IDs to strings.

Addressed code review comments and made some bug fixes.

Comment 7

[Doug Sherk (:drs) (inactive)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=cffd58590c65

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 577856 [details] [diff] [review]
Patch v1.1, port blocklist vendor/device IDs to strings.

Review of attachment 577856 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Are you going to add tests for non-hexadecimal device and vendor IDs here, or in another bug?

::: widget/src/xpwidgets/GfxDriverInfo.cpp
@@ +96,4 @@
>  
> +    mDevices->SetLength(aOrig.mDevices->Length());
> +    for (PRUint32 i = 0; i < aOrig.mDevices->Length(); i++)
> +      (*mDevices)[i] = (*aOrig.mDevices)[i];

Does nsTArray<nsString>'s operator= not handle this for us?

::: widget/src/xpwidgets/GfxInfoBase.cpp
@@ +773,5 @@
> +  PRInt32 status;
> +  if (aDriverInfo.Length())
> +    status = FindBlocklistedDeviceInList(aDriverInfo, aSuggestedVersion, aFeature, os);
> +  else
> +  {

Here you should put {} around the if body, and cuddle the else "} else {".

Comment 9

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.2, port blocklist vendor/device IDs to strings.

Addressed code review comments, +r carried.

Comment 10

[Doug Sherk (:drs) (inactive)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=abe278c327db

Comment 11

[Doug Sherk (:drs) (inactive)]

https://tbpl.mozilla.org/?rev=fe937bac6e75

Comment 12

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8c075fee9be4

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Backed out from beta only:
http://hg.mozilla.org/releases/mozilla-beta/rev/ff5f2055aba4
See bug 711656 for an explanation.