Closed Bug 704710 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Assignee: nobody → dsherk
Attachment #576383 - Flags: review?(bjacob)
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
Depends on: 699482
Blocks: 689598
Blocks: 668008
No longer blocks: 689598
Blocks: 689598
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.
Attachment #576383 - Flags: review?(bjacob) → review?(joe)
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?
Attachment #576383 - Flags: review?(joe) → review+
Oh, one additional question: you have nsString.Equals() all over the place - does operator== not work?
Addressed code review comments and made some bug fixes.
Attachment #576383 - Attachment is obsolete: true
Attachment #577856 - Flags: review?(joe)
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 {".
Attachment #577856 - Flags: review?(joe) → review+
Blocks: 706245
No longer blocks: 668008
Blocks: 706702
Addressed code review comments, +r carried.
Attachment #577856 - Attachment is obsolete: true
Attachment #578494 - Flags: review+
https://tbpl.mozilla.org/?rev=fe937bac6e75
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 711656
Depends on: 704143
No longer depends on: 704143
No longer depends on: 711656
You need to log in before you can comment on or make changes to this bug.