Last Comment Bug 704710 - Refactor GfxDriverInfo/GfxInfo(Base) to support string device and vendor IDs
: Refactor GfxDriverInfo/GfxInfo(Base) to support string device and vendor IDs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Doug Sherk (:drs) (inactive)
:
Mentors:
Depends on: 699482
Blocks: 689598 706245 706702
  Show dependency treegraph
 
Reported: 2011-11-22 18:09 PST by Doug Sherk (:drs) (inactive)
Modified: 2012-03-10 21:27 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0, port blocklist vendor/device IDs to strings. (72.38 KB, patch)
2011-11-22 18:29 PST, Doug Sherk (:drs) (inactive)
joe: review+
Details | Diff | Review
Patch v1.1, port blocklist vendor/device IDs to strings. (77.07 KB, patch)
2011-11-29 21:12 PST, Doug Sherk (:drs) (inactive)
joe: review+
Details | Diff | Review
Patch v1.2, port blocklist vendor/device IDs to strings. (78.87 KB, patch)
2011-12-01 21:41 PST, Doug Sherk (:drs) (inactive)
bugzilla: review+
Details | Diff | Review

Description Doug Sherk (:drs) (inactive) 2011-11-22 18:09:05 PST
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) 2011-11-22 18:29:14 PST
Created attachment 576383 [details] [diff] [review]
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) 2011-11-22 18:30:23 PST
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) 2011-11-22 18:49:55 PST
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) 2011-11-24 10:32:32 PST
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) 2011-11-24 10:35:15 PST
Oh, one additional question: you have nsString.Equals() all over the place - does operator== not work?
Comment 6 Doug Sherk (:drs) (inactive) 2011-11-29 21:12:52 PST
Created attachment 577856 [details] [diff] [review]
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) 2011-11-29 21:34:40 PST
Try push: https://tbpl.mozilla.org/?tree=Try&rev=cffd58590c65
Comment 8 Joe Drew (not getting mail) 2011-11-30 11:30:27 PST
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) 2011-12-01 21:41:25 PST
Created attachment 578494 [details] [diff] [review]
Patch v1.2, port blocklist vendor/device IDs to strings.

Addressed code review comments, +r carried.
Comment 10 Doug Sherk (:drs) (inactive) 2011-12-01 21:48:28 PST
Try push: https://tbpl.mozilla.org/?tree=Try&rev=abe278c327db
Comment 11 Doug Sherk (:drs) (inactive) 2011-12-14 23:54:53 PST
https://tbpl.mozilla.org/?rev=fe937bac6e75
Comment 12 Doug Sherk (:drs) (inactive) 2011-12-15 01:22:32 PST
https://hg.mozilla.org/mozilla-central/rev/8c075fee9be4
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-03-08 15:15:11 PST
Backed out from beta only:
http://hg.mozilla.org/releases/mozilla-beta/rev/ff5f2055aba4
See bug 711656 for an explanation.

Note You need to log in before you can comment on or make changes to this bug.