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.

saari: can you r= it

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

Comment on attachment 49873 [details] [diff] [review] Properly hand back errors plus some code cleanup ok, r=saari

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?

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...?

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 on attachment 65679 [details] [diff] [review] New patch to cleanup stuff and properly hand back errors er, this patch is backwards.

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

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++)

fixed