100470 - GIF decoder should properly return errors decoding

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P4)

Description

[Stuart Parmenter]

the GIF decoder needs to return error codes from WriteFrom so that bad GIFs get
properly booted from the cache, we don't waste time downloading them, etc.

Comment 1

[Stuart Parmenter]

Attachment: Properly hand back errors plus some code cleanup

Comment 2

[Stuart Parmenter]

saari: can you r= it

Comment 3

[jag (Peter Annema)]

Comment on attachment 49873 [details] [diff] [review]
Properly hand back errors plus some code cleanup

r=jag

Comment 4

[saari (gone)]

Comment on attachment 49873 [details] [diff] [review]
Properly hand back errors plus some code cleanup

ok, r=saari

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 49873 [details] [diff] [review]
Properly hand back errors plus some code cleanup

>     if (gs->gathered < max)
>-        return 1;               /* Let imglib generic code decide */
>+        return PR_TRUE;         /* Let imglib generic code decide */
>     else
>-        return 0;               /* No more data until timeout expires */
>+        return PR_FALSE;        /* No more data until timeout expires */

Else after return: that's a paddling!

>+
>+#if 0
>     if (gs->destroy_pending &&
>         ((state == gif_done) || (state == gif_error) || (state == gif_oom))) {
> 
>@@ -635,6 +631,7 @@
>         //  ic->imgdcb->ImgDCBHaveImageAll();
>         // XXX: Do Have all callback
>     }
>+#endif

Is this code coming back in the next few days?  If not, please just remove it
rather than #defining it out.

>+static PRStatus
> gif_clear_screen(gif_struct *gs)
> {
...
>-            return NS_ERROR_FAILURE; // XXX: should be out of mem error
>+            return PR_FAILURE;

Why PRStatus and not PRBool?

Why not nsresult and NS_ERROR_OUT_OF_MEMORY?

> 
>+
>+
> // XXX pav this code doesn't actually get called does it ?
> 
>+
>+

Why the bonus whitespace?


>+#if 0
>+            // XXX handle colormap stuff
>+
>             //cmap->map = gs->is_local_colormap_defined ?
>             //    gs->local_colormap : gs->global_colormap;
> 
>@@ -848,9 +848,10 @@
>                 //    return MK_IMAGE_LOSSAGE;
>                 //}
>             }
>+#endif

Again with the #if 0!

> #endif
> #endif /* M12N */

(We still have M12N defines in here?)

>+#if 0
>             if (gs->images_decoded == 0) {
>                 // XXX: do image size callback
>                 //if(ic->imgdcb){
>@@ -1330,9 +1308,9 @@
>                 //    break;
>                 //}
>             }
>+#endif

Etc.


>+                        if (NULL == BlockAllocCat(hold, gs->gathered,
>+                                                  (char*)p,
>+                                                  gather_remaining)) {
>+                            gs->state = gif_oom;
>+                            break;
>+                        }

Explicit compare against NULL seems out of character for this file.

>             //ILTRACE(1,("il:gif: reached oom state"));
>-            return NS_ERROR_FAILURE; // XXX should be out of mem error
>+            return PR_FAILURE;
>             break;

Again: why not nsresult and NS_ERROR_OUT_OF_MEMORY?

>-  GIFInit(
>-    mGIFStruct,
>+  GIFInit(mGIFStruct,
>     this,
>     NewPixmap,
>     BeginGIF,

Underindented parameters (and why not put them on the same line?).

>-  NS_METHOD ProcessData(unsigned char *data, PRUint32 count);
>+  nsresult ProcessData(unsigned char *data, PRUint32 count, PRUint32 *writeCount);
> 
>   NS_METHOD FlushImageData();

You don't love NS_METHOD?  What about FlushImageData?

Comment 6

[Gagan]

0.9.6 is gone. Pav is this going to make it for 0.9.7? Seems like it just need a
r/sr again with shaver's suggestions...?

Comment 7

[Stuart Parmenter]

Attachment: New patch to cleanup stuff and properly hand back errors

Newer/cleaner patch for handing back errors.  diff -uw to avoid the numerous
lines of reindenting 2 spaces to match the rest of the 4 space'd file.

Comment 8

[Stuart Parmenter]

Comment on attachment 65679 [details] [diff] [review]
New patch to cleanup stuff and properly hand back errors

er, this patch is backwards.

Comment 9

[Stuart Parmenter]

Attachment: Correct patch

Comment 10

[Stuart Parmenter]

Attachment: merged-to-the-tip

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 66353 [details] [diff] [review]
merged-to-the-tip

r=bryner

Comment 12

[Scott MacGregor]

Comment on attachment 66353 [details] [diff] [review]
merged-to-the-tip

sr=mscott

don't you want i to be a PRIn32 or a PRUint32 instead of a raw "int"? 

-	     for (i=0; i < gs->global_colormap_size; i++, map++) 
+	     for (int i=0; i < gs->global_colormap_size; i++, map++)

Comment 13

[Stuart Parmenter]

fixed