625160 - Need to load graphics blocklist entries from blocklist service

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Joe Drew (not getting mail)]

Attachment: part 1: Split out the Windows DriverInfo structure into a separate file

Comment 2

[Joe Drew (not getting mail)]

Attachment: part 2: Create a base class of Windows' GfxInfo that will let us override GetFeatureStatus and GetFeatureSuggestedDriverVersion

Comment 3

[Joe Drew (not getting mail)]

Attachment: part 3: Let prefs, one per feature, override the return value of GetFeatureStatus and GetFeatureSuggestedDriverVersion

Comment 4

[Joe Drew (not getting mail)]

Attachment: part 4: Register for the blocklist services' gfxInfo section, and parse that XML into a GfxDriverInfo

Comment 5

[Joe Drew (not getting mail)]

Attachment: part 5: Evaluate the downloaded and parsed blacklist, and set the override prefs based on it

Comment 6

[Joe Drew (not getting mail)]

This works on Windows. Still to do: Get it compiling on Mac and Android (our other GfxInfo implementations), and write tests.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

drive-by note: from talking to Mossop, I think this data needs to be cached -- we'll only get the blocklist data once per day, which means that the code will only trigger once per day.  If someone quits and restarts firefox, they still need to have up to date blocklist info.

It might be enough for 4.0.0 to just have a "blocked by blocklist" pref, which we'd always flip on/off based on the blocklist status (to let us remove things from the blocklist if they were added wrongly).  That would cover most use cases, but wouldn't cover "I quit firefox and turned off my PC and installed a new video card, then fired up firefox again and now it's crashing".  But it should be ok to get to that case in a followup...

Comment 8

[Joe Drew (not getting mail)]

Drive-by reply: already handled!

Comment 9

[Joe Drew (not getting mail)]

I've made a bunch of changes (getting stuff to compile on Mac, Linux, etc), and I honestly don't remember which patches got changed, so I'm going to reupload all of them. Sorry for the bugspam!

Comment 10

[Joe Drew (not getting mail)]

Attachment: part 1: Split out the Windows DriverInfo structure into a separate file

Comment 11

[Joe Drew (not getting mail)]

Attachment: part 2: Create a base class for all GfxInfo implementations that will let us override GetFeatureStatus and GetFeatureSuggestedDriverVersion

Comment 12

[Joe Drew (not getting mail)]

Attachment: part 3: Let prefs, one per feature, override the return value of GetFeatureStatus and GetFeatureSuggestedDriverVersion

Comment 13

[Joe Drew (not getting mail)]

Attachment: part 4: Register for the blocklist services' gfxInfo section, and parse that XML into a GfxDriverInfo

Comment 14

[Joe Drew (not getting mail)]

Attachment: part 5: Evaluate the downloaded and parsed blacklist, and set the override prefs based on it

Comment 15

[Joe Drew (not getting mail)]

Attachment: part 6: move all driver/device/etc spoofing initialization to Init(), to allow it to be overridden in future patches

Comment 16

[Joe Drew (not getting mail)]

Attachment: part 7: create a debug-only nsIGfxInfoDebug interface that lets us programmatically spoof OS version, device, etc

Comment 17

[Joe Drew (not getting mail)]

Attachment: part 8: add gfx blacklist tests

These are not exhaustive, obviously, but they're a good first step.

Comment 18

[Joe Drew (not getting mail)]

A very important note: These patches implement the downloaded blacklist ONLY ON WINDOWS. Mac/Android support can come later if it's necessary.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 504359 [details] [diff] [review]
part 1: Split out the Windows DriverInfo structure into a separate file

I'd prefer to have the enums namespaced and then we can "using" it to avoid have to scatter the prefix all around.

Comment 20

[Joe Drew (not getting mail)]

Attachment: part 4: Register for the blocklist services' gfxInfo section, and parse that XML into a GfxDriverInfo

Accidentally uploaded the wrong patch last time.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 504499 [details] [diff] [review]
part 4: Register for the blocklist services' gfxInfo section, and parse that XML into a GfxDriverInfo

[snip]

>+static PRUint32
>+BlacklistHexToInt(const nsAString& aHex)
>+{
>+  PRInt32 err;
>+  nsCAutoString hex = NS_LossyConvertUTF16toASCII(aHex);
>+  PRInt32 value = hex.ToInteger(&err, 16);
>+  if (NS_FAILED(err))
>+    return 0;
>+  return (PRUint32) value;
>+}

Let's avoid the conversion to ASCII if we can.

>+
>+static PRUint32*
>+BlacklistDevicesToDeviceFamily(nsIDOMNodeList* aDevices)
>+{
>+  // Collect the <device> nodes.
>+  nsCOMArray<nsIDOMNode> deviceNodes;
>+
>+  PRUint32 length;
>+  if (NS_FAILED(aDevices->GetLength(&length)))
>+    return nsnull;
>+
>+  for (PRUint32 i = 0; i < length; ++i) {
>+    nsCOMPtr<nsIDOMNode> device;
>+    if (NS_FAILED(aDevices->Item(i, getter_AddRefs(device))))
>+      continue;
>+
>+    nsAutoString deviceNodeName;
>+    if (NS_FAILED(device->GetNodeName(deviceNodeName)) ||
>+        deviceNodeName != NS_LITERAL_STRING("device"))
>+      continue;
>+
>+    deviceNodes.AppendObject(device);
>+  }
>+
>+  // For each <device>, get its device ID, and return a freshly-allocated array
>+  // with the contents of that array.
>+  nsAutoArrayPtr<PRUint32> deviceIds(new PRUint32[deviceNodes.Count() + 1]);
>+  memset(deviceIds, 0, sizeof(PRUint32) * (deviceNodes.Count() + 1));
>+
>+  for (PRInt32 i = 0; i < deviceNodes.Count(); ++i) {
>+    nsAutoString deviceValue;
>+    if (!BlacklistNodeToTextValue(deviceNodes[i], deviceValue))
>+      continue;
>+
>+    PRUint32 deviceId = BlacklistHexToInt(deviceValue);
>+    if (!deviceId)
>+      continue;
>+
>+    deviceIds[i] = deviceId;
>+  }
>+
>+  return deviceIds.forget();
>+}
>+

[snip]

>+    return GfxDriverInfo::DRIVER_BETWEEN_INCLUSIVE;
>+  else if (op == NS_LITERAL_STRING("BETWEEN_INCLUSIVE_START"))
>+    return GfxDriverInfo::DRIVER_BETWEEN_INCLUSIVE_START;
>+
>+  return GfxDriverInfo::DRIVER_LESS_THAN;
>+}

These comparison functions should handle errors better.
Have them return an invalid id.

>+
>+/*
>+
>+<gfxBlacklistEntry>
>+  <os>WINNT 6.0</os>
>+  <vendor>0x8086</vendor>
>+  <devices>
>+    <device>0x2582</device>
>+    <device>0x2782</device>
>+  </devices>
>+  <feature> DIRECT3D_10_LAYERS </feature>
>+  <featureStatus> BLOCKED_DRIVER_VERSION </featureStatus>
>+  <driverVersion> 8.52.322.2202 </driverVersion>
>+  <driverVersionComparator> LESS_THAN_OR_EQUAL </driverVersionComparator>
>+</gfxBlacklistEntry>
>+
>+*/
>+static bool
>+BlacklistEntryToDriverInfo(nsIDOMNode* aBlacklistEntry,
>+                           GfxDriverInfo& aDriverInfo)
>+{

[snip]

>+}

Please redo BlacklistEntryToDriverInfo() this to use something like getElementByTagName()


[snip]

>+GfxInfoBase::GfxInfoBase()
>+{
>+  mBlacklistObserver = new BlacklistObserver(this);
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();

I'd rather the observer have a reference to GfxInfo then vice-versa.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 504365 [details] [diff] [review]
part 7: create a debug-only nsIGfxInfoDebug interface that lets us programmatically spoof OS version, device, etc

I'm not sure it's worth spoofing the windows version is it?

Comment 23

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #22)
> Comment on attachment 504365 [details] [diff] [review]
> part 7: create a debug-only nsIGfxInfoDebug interface that lets us
> programmatically spoof OS version, device, etc
> 
> I'm not sure it's worth spoofing the windows version is it?

I guess you want this 'cause the xml contains OS.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 504366 [details] [diff] [review]
part 8: add gfx blacklist tests

Shouldn't this spoof different things and see how the blacklist interacts?

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 504365 [details] [diff] [review]
part 7: create a debug-only nsIGfxInfoDebug interface that lets us programmatically spoof OS version, device, etc

I can't think of a better way to do this.

Comment 26

[Dave Townsend [:mossop]]

Comment on attachment 504499 [details] [diff] [review]
part 4: Register for the blocklist services' gfxInfo section, and parse that XML into a GfxDriverInfo

Will just defer to Jeff on subsequent reviews of this. Aside from the few minor issues below this all looks fine to me.

>diff --git a/widget/src/xpwidgets/GfxInfoBase.cpp b/widget/src/xpwidgets/GfxInfoBase.cpp

>+// <foo>Hello</foo> - "Hello" is stored as a child text node of the foo node.
>+static bool
>+BlacklistNodeToTextValue(nsIDOMNode *aBlacklistNode, nsAString& aValue)
>+{
>+  nsCOMPtr<nsIDOMNode> child;
>+  if (NS_FAILED(aBlacklistNode->GetFirstChild(getter_AddRefs(child))) || !child)
>+    return false;
>+
>+  PRUint16 type = 0;
>+  if (NS_FAILED(child->GetNodeType(&type)) || type != nsIDOMNode::TEXT_NODE)
>+    return false;
>+
>+  if (NS_FAILED(child->GetNodeValue(aValue)))
>+    return false;
>+
>+  return true;

This isn't quite right as it ignores the (albeit unlikely) possibility of multiple child text nodes. Best way to go is probably to just QI to nsIDOMElement and use GetTextContent.

>+static PRUint32*
>+BlacklistDevicesToDeviceFamily(nsIDOMNodeList* aDevices)
>+{
>+  // Collect the <device> nodes.
>+  nsCOMArray<nsIDOMNode> deviceNodes;
>+
>+  PRUint32 length;
>+  if (NS_FAILED(aDevices->GetLength(&length)))
>+    return nsnull;
>+
>+  for (PRUint32 i = 0; i < length; ++i) {
>+    nsCOMPtr<nsIDOMNode> device;
>+    if (NS_FAILED(aDevices->Item(i, getter_AddRefs(device))))
>+      continue;
>+
>+    nsAutoString deviceNodeName;
>+    if (NS_FAILED(device->GetNodeName(deviceNodeName)) ||
>+        deviceNodeName != NS_LITERAL_STRING("device"))
>+      continue;
>+
>+    deviceNodes.AppendObject(device);
>+  }
>+
>+  // For each <device>, get its device ID, and return a freshly-allocated array
>+  // with the contents of that array.
>+  nsAutoArrayPtr<PRUint32> deviceIds(new PRUint32[deviceNodes.Count() + 1]);
>+  memset(deviceIds, 0, sizeof(PRUint32) * (deviceNodes.Count() + 1));
>+
>+  for (PRInt32 i = 0; i < deviceNodes.Count(); ++i) {
>+    nsAutoString deviceValue;
>+    if (!BlacklistNodeToTextValue(deviceNodes[i], deviceValue))
>+      continue;

Probably should trim this string for whitespace same as elsewhere.

>+NS_IMETHODIMP
>+BlacklistObserver::Observe(nsISupports* aSubject, const char* aTopic,
>+                           const PRUnichar* aData)
>+{
>+  if (strcmp(aTopic, "blocklist-data-gfxItems") == 0) {
>+    nsCOMPtr<nsIDOMNode> gfxItems = do_QueryInterface(aSubject);
>+    if (gfxItems) {
>+      nsCOMPtr<nsIDOMNodeList> blacklistEntries;
>+      gfxItems->GetChildNodes(getter_AddRefs(blacklistEntries));
>+      if (blacklistEntries) {
>+        nsTArray<GfxDriverInfo> driverInfo;
>+        BlacklistEntriesToDriverInfo(blacklistEntries, driverInfo);

I presume you do something with this in a later patch.

>+GfxInfoBase::GfxInfoBase()
>+{
>+  mBlacklistObserver = new BlacklistObserver(this);
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+  if (os) {
>+    os->AddObserver(mBlacklistObserver, "blocklist-data-gfxItems", PR_FALSE);
>+  }

Personally I would probably just make GfxInfoBase implement nsIObserver to save having the extra class for this. Not terribly bothered either way though.

Comment 27

[Dave Townsend [:mossop]]

Comment on attachment 504366 [details] [diff] [review]
part 8: add gfx blacklist tests

There is certainly more that could be tested here (different comparisons f.e.) however to start with could we get some comments at the top of each test saying what it is actually testing. My brain is getting fuzzy just trying to figure it out for myself.

Comment 28

[Joe Drew (not getting mail)]

(In reply to comment #21)
> Comment on attachment 504499 [details] [diff] [review]
> part 4: Register for the blocklist services' gfxInfo section, and parse that
> XML into a GfxDriverInfo
> 
> [snip]
> 
> >+static PRUint32
> >+BlacklistHexToInt(const nsAString& aHex)
> >+{
> >+  PRInt32 err;
> >+  nsCAutoString hex = NS_LossyConvertUTF16toASCII(aHex);
> >+  PRInt32 value = hex.ToInteger(&err, 16);
> >+  if (NS_FAILED(err))
> >+    return 0;
> >+  return (PRUint32) value;
> >+}
> 
> Let's avoid the conversion to ASCII if we can.

We can't :(

Comment 29

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #28)
> (In reply to comment #21)
> > Comment on attachment 504499 [details] [diff] [review]
> > 
> > Let's avoid the conversion to ASCII if we can.
> 
> We can't :(

Why not? nsAutoString seems to have ToInteger()

Comment 30

[Joe Drew (not getting mail)]

nsAString is not the same as nsAutoString, sadly.

Comment 31

[Joe Drew (not getting mail)]

Oh - OK. I can avoid the conversion, but not the copy.

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Another drive-by suggestion, but perhaps too late: implement this all in JS for simplicity, use jsctypes for getting system info?

Comment 33

[Joe Drew (not getting mail)]

(In reply to comment #24)
> Comment on attachment 504366 [details] [diff] [review]
> part 8: add gfx blacklist tests
> 
> Shouldn't this spoof different things and see how the blacklist interacts?

It does do this, but unfortunately the tests weren't labelled correctly. New tests coming up.

Comment 34

[Joe Drew (not getting mail)]

(In reply to comment #32)
> Another drive-by suggestion, but perhaps too late: implement this all in JS for
> simplicity, use jsctypes for getting system info?

I thought about it, but didn't do it, maybe for no good reason. It's too late now, yes.

Comment 35

[Joe Drew (not getting mail)]

Attachment: part 1: Split out the Windows DriverInfo structure into a separate file, v2

Comment 36

[Joe Drew (not getting mail)]

Attachment: part 2: create GfxInfoBase, v2

Carrying forward r+

Comment 37

[Joe Drew (not getting mail)]

Attachment: part 3: add prefs, v2

carrying forward r+

Comment 38

[Joe Drew (not getting mail)]

Attachment: part 4: parse the downloaded XML/DOM tree, v2

Addressed review comments.

Comment 39

[Joe Drew (not getting mail)]

Attachment: part 5: evaluate the downloaded and parsed blacklist, and set the override prefs based on it

Comment 40

[Joe Drew (not getting mail)]

Attachment: part 6: move all spoofing to Init

Comment 41

[Joe Drew (not getting mail)]

Attachment: part 7: create nsIGfxInfoDebug

Jeff reviewed this patch earlier; Benoit still needs to, though.

Comment 42

[Joe Drew (not getting mail)]

Attachment: part 8: add tests v2

This patch adds comments to every test, and adds a test that makes sure we're setting and removing the prefs that save the blocking decisions for startup.

Comment 43

[Dave Townsend [:mossop]]

Comment on attachment 504979 [details] [diff] [review]
part 8: add tests v2

Looks good apart from a few comment errors. I think even for this first landing you should also add a test for the case where only the vendor ID differs and a couple of different comparison cases, I would hazard a guess that BETWEEN_INCLUSIVE_START, EQUAL and GREATER_THAN_OR_EQUAL are liable to be the most common other cases that we block on.

One thing I suddenly realised is that in all your code you've used the term "blacklist" when we generally refer to the service as a "blocklist". Maybe not a big deal and perhaps a bit late to switch it everywhere but it does mean we're a little inconsistent.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Device.js b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Device.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_Device.js
>@@ -0,0 +1,71 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+// Test whether a machine which differs only on device ID, but otherwise
>+// exactly matches the blacklist entry, is successfully blocked.
>+// Uses test_gfxBlacklist.xml

Seems to actually check that a device with a different device ID is not blocked.

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_DriverNew.js b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_DriverNew.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_DriverNew.js
>@@ -0,0 +1,72 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+// Test whether a new-enough driver bypasses the blacklist, even if the rest of
>+// the attributes match the blacklist entry.
>+// Uses test_gfxBlacklist.xml
>+
>+do_load_httpd_js();
>+
>+var gTestserver = null;
>+
>+function load_blocklist(file) {
>+  Services.prefs.setCharPref("extensions.blocklist.url", "http://localhost:4444/data/" + file);
>+  var blocklist = Cc["@mozilla.org/extensions/blocklist;1"].
>+                  getService(Ci.nsITimerCallback);
>+  blocklist.notify(null);
>+}
>+
>+// Performs the initial setup
>+function run_test() {
>+  try {
>+    var gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
>+  } catch (e) {
>+    do_test_finished();
>+    return;
>+  }
>+
>+  // We can't do anything if we can't spoof the stuff we need.
>+  // We can't do anything if we can't spoof the stuff we need.

Probably only need this comment once (also in test_gfxBlacklist_OK.js)

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_OS.js b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_OS.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_OS.js
>@@ -0,0 +1,72 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+// Test whether a machine which differs only on OS version, but otherwise
>+// exactly matches the blacklist entry, is successfully blocked.
>+// Uses test_gfxBlacklist.xml

Actually checks that it isn't blocked

Comment 44

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 504974 [details] [diff] [review]
part 4: parse the downloaded XML/DOM tree, v2

I wouldn't complain about the separating out the observer business from the parsing, but I expect that's not all that pleasant to do.

>-void
>+nsresult
> GfxInfo::Init()
> {
>   NS_TIME_FUNCTION;
> 
>+  nsresult rv = GfxInfoBase::Init();
>+

These return values make me sad, but perhaps there's not much that can be done. Please add a comment about the fact that they're needed for the construction stuff.

>+GfxDriverInfo::GfxDriverInfo()
>+  : mOperatingSystem(DRIVER_OS_UNKNOWN),
>+    mAdapterVendor(allAdapterVendors),
>+    mDevices(allDevices),
>+    mDeleteDevices(false),
>+    mFeature(allFeatures),
>+    mFeatureStatus(nsIGfxInfo::FEATURE_NO_INFO),
>+    mComparisonOp(DRIVER_LESS_THAN),

Wrong comparison op.

>+static PRUint32
>+BlacklistHexToInt(const nsAString& aHex)
>+{
>+  PRInt32 err;
>+  nsAutoString hex(aHex);
>+  PRInt32 value = hex.ToInteger(&err, 16);

Might as well add a comment about nsAString not supporting ToInteger.

>+  if (NS_FAILED(err))
>+    return 0;
>+  return (PRUint32) value;
>+}
>+
>+static PRUint32*
>+BlacklistDevicesToDeviceFamily(nsIDOMNodeList* aDevices)
>+{
>+  // Collect and count the <device> nodes.

Fix this comment.

>+  nsCOMArray<nsIDOMNode> deviceNodes;
>+
>+  PRUint32 length;
>+  if (NS_FAILED(aDevices->GetLength(&length)))
>+    return nsnull;
>+


>+static bool
>+BlacklistNodeGetFirstChildWithTagName(nsIDOMElement *element,
>+                                      const nsAString& tagname,
>+                                      nsIDOMNode** firstchild)
>+{

This name has more information than necessary. I'd trim it down to:
BlacklistNodeGetChildByName. You can add a comment in the implementation that
it arbitrarily chooses the first one.

>+  nsCOMPtr<nsIDOMNodeList> nodelist;
>+  if (NS_FAILED(element->GetElementsByTagName(tagname,
>+                                              getter_AddRefs(nodelist))) ||
>+      !nodelist) {
>+    return false;
>+  }
>+
>+  PRUint32 length = 0;
>+  if (NS_FAILED(nodelist->GetLength(&length)) || length == 0)
>+    return false;
>+

Do we really need to check the length here? I'd expect the next call to fail if length == 0

Comment 45

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/3706c8ac95b7
http://hg.mozilla.org/mozilla-central/rev/cce78c43034a
http://hg.mozilla.org/mozilla-central/rev/4e348632d5e4
http://hg.mozilla.org/mozilla-central/rev/839ddef92d3f
http://hg.mozilla.org/mozilla-central/rev/aa85b04aa9ef
http://hg.mozilla.org/mozilla-central/rev/ef843567f03a
http://hg.mozilla.org/mozilla-central/rev/e1b6a5426f31
http://hg.mozilla.org/mozilla-central/rev/80c4d7317c29

Comment 46

[Makoto Kato [:m_kato]]

bustage on Android

http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1295487973.1295488202.19222.gz

/builds/slave/cen-mb-br-andrd-r7-bld/build/mozilla-central/widget/src/android/GfxInfo.cpp:57: error: no 'nsrefcnt mozilla::widget::GfxInfo::AddRef()' member function declared in class 'mozilla::widget::GfxInfo'
/builds/slave/cen-mb-br-andrd-r7-bld/build/mozilla-central/widget/src/android/GfxInfo.cpp:57: error: no 'nsrefcnt mozilla::widget::GfxInfo::Release()' member function declared in class 'mozilla::widget::GfxInfo'
/builds/slave/cen-mb-br-andrd-r7-bld/build/mozilla-central/widget/src/android/GfxInfo.cpp:57: error: no 'nsresult mozilla::widget::GfxInfo::QueryInterface(const nsIID&, void**)' member function declared in class 'mozilla::widget::GfxInfo'
make[6]: *** [GfxInfo.o] Error 1

Comment 47

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

Attachment: Remove old allFeatures=-1

Comment 48

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

http://hg.mozilla.org/mozilla-central/rev/80b4174a2608

Comment 49

[[e-mail]]

Comment on attachment 504972 [details] [diff] [review]
part 3: add prefs, v2

>+static bool
>+GetPrefValueForDriverVersion(nsACString& aVersion)
...
>+static void
>+SetPrefValueForDriverVersion(const nsAString& aVersion)
This narrow/wide mismatch was a source for confusion. I'm filing a bug.