Merge lp:~gordallott/unity/fix-937421 into lp:unity

[Gord Allott]

Check pixbuf before dereferencing.

safety wrapper around the gdkpixbuf sent to the object

no test as I can't reproduce the issue, without the pixbuf that was causing the issue, we can't write a succesful test

UNBLOCK

[Tim Penhey (thumper)]

Is there any way we can fake a bad pixbuf for this function?

If not, how about extract that content into a stand alone function, and test that in a unit test.

If the function content is the same as the LaunchadIcon.cpp, they should both use the same function.

review: Needs Information

[Gord Allott (gordallott)]

Without knowing precicely what the bad pixbuf is, a test is meaningless, it may or may not be testing what you thought it was and it certainly does not verrify that the bug itself is fixed

What we need is an application guarunteed to cause the bug, so we can look at the real reason as to why a bad pixbuf is generated in the first place and potentially build ap tests using that application.

I really can't think of a way of testing this change that doesn't basically just boil down to writing EXPECT_EQ(GDK_IS_PIXBUF(NULL), FALSE);

[Tim Penhey (thumper)]

OK. Fair enough. I can't think of a reasonable test for this either. We do say that sometimes we need to use our discretion, and to be reasonable. I think this is one of those cases.

I approve this landing without an explicit test.

review: Approve

=== modified file 'plugins/unityshell/src/HudIconTextureSource.cpp'
--- plugins/unityshell/src/HudIconTextureSource.cpp	2012-02-02 18:01:23 +0000
+++ plugins/unityshell/src/HudIconTextureSource.cpp	2012-02-28 14:18:23 +0000
@@ -26,6 +26,10 @@
 #include <Nux/Nux.h>
 #include <NuxCore/Logger.h>
 
+namespace
+{
+  nux::logging::Logger logger("unity.hud.HudIconTextureSource");
+}
 
 namespace unity
 {
@@ -44,46 +48,54 @@
 
 void HudIconTextureSource::ColorForIcon(GdkPixbuf* pixbuf)
 {
-  unsigned int width = gdk_pixbuf_get_width(pixbuf);
-  unsigned int height = gdk_pixbuf_get_height(pixbuf);
-  unsigned int row_bytes = gdk_pixbuf_get_rowstride(pixbuf);
-  
-  long int rtotal = 0, gtotal = 0, btotal = 0;
-  float total = 0.0f;
-  
-  guchar* img = gdk_pixbuf_get_pixels(pixbuf);
-  
-  for (unsigned int i = 0; i < width; i++)
+  if (GDK_IS_PIXBUF(pixbuf))
   {
-    for (unsigned int j = 0; j < height; j++)
+    unsigned int width = gdk_pixbuf_get_width(pixbuf);
+    unsigned int height = gdk_pixbuf_get_height(pixbuf);
+    unsigned int row_bytes = gdk_pixbuf_get_rowstride(pixbuf);
+    
+    long int rtotal = 0, gtotal = 0, btotal = 0;
+    float total = 0.0f;
+    
+    guchar* img = gdk_pixbuf_get_pixels(pixbuf);
+    
+    for (unsigned int i = 0; i < width; i++)
     {
-      guchar* pixels = img + (j * row_bytes + i * 4);
-      guchar r = *(pixels + 0);
-      guchar g = *(pixels + 1);
-      guchar b = *(pixels + 2);
-      guchar a = *(pixels + 3);
-      
-      float saturation = (MAX(r, MAX(g, b)) - MIN(r, MIN(g, b))) / 255.0f;
-      float relevance = .1 + .9 * (a / 255.0f) * saturation;
-      
-      rtotal += (guchar)(r * relevance);
-      gtotal += (guchar)(g * relevance);
-      btotal += (guchar)(b * relevance);
-      
-      total += relevance * 255;
+      for (unsigned int j = 0; j < height; j++)
+      {
+        guchar* pixels = img + (j * row_bytes + i * 4);
+        guchar r = *(pixels + 0);
+        guchar g = *(pixels + 1);
+        guchar b = *(pixels + 2);
+        guchar a = *(pixels + 3);
+        
+        float saturation = (MAX(r, MAX(g, b)) - MIN(r, MIN(g, b))) / 255.0f;
+        float relevance = .1 + .9 * (a / 255.0f) * saturation;
+        
+        rtotal += (guchar)(r * relevance);
+        gtotal += (guchar)(g * relevance);
+        btotal += (guchar)(b * relevance);
+        
+        total += relevance * 255;
+      }
     }
-  }
-  
-  nux::color::RedGreenBlue rgb(rtotal / total,
-                               gtotal / total,
-                               btotal / total);
-  nux::color::HueSaturationValue hsv(rgb);
-  
-  if (hsv.saturation > 0.15f)
-    hsv.saturation = 0.65f;
-  
-  hsv.value = 0.90f;
-  bg_color = nux::Color(nux::color::RedGreenBlue(hsv));
+    
+    nux::color::RedGreenBlue rgb(rtotal / total,
+                                 gtotal / total,
+                                 btotal / total);
+    nux::color::HueSaturationValue hsv(rgb);
+    
+    if (hsv.saturation > 0.15f)
+      hsv.saturation = 0.65f;
+    
+    hsv.value = 0.90f;
+    bg_color = nux::Color(nux::color::RedGreenBlue(hsv));
+  }
+  else
+  {
+    LOG_ERROR(logger) << "Pixbuf (" << pixbuf << ") passed is non valid";
+    bg_color = nux::Color(255,255,255,255);
+  }
 }
 
 nux::Color HudIconTextureSource::BackgroundColor()
@@ -107,4 +119,5 @@
 }
 
 }
-}
\ No newline at end of file
+}
+