Last Comment Bug 549468 - Add basic support for .ico icon encoder
: Add basic support for .ico icon encoder
Status: RESOLVED FIXED
[sec-assigned:dveditz]
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 Windows 7
: -- normal with 3 votes (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on: 600556 670466 682201 750441
Blocks: win7support 549472 668068
  Show dependency treegraph
 
Reported: 2010-03-01 15:21 PST by Jim Mathies [:jimm]
Modified: 2012-08-15 16:19 PDT (History)
20 users (show)
dveditz: sec‑review? (dveditz)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Intermediate patch for .ico icon encoder (31.58 KB, text/plain)
2011-07-11 19:39 PDT, Brian R. Bondy [:bbondy]
no flags Details
Intermediate patch for .ico icon encoder (31.58 KB, patch)
2011-07-11 19:43 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Finalized patch for ico encoder (34.61 KB, patch)
2011-07-12 20:29 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Finalized patch for ico encoder (34.61 KB, patch)
2011-07-14 12:56 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch for working ICO encoder (34.58 KB, patch)
2011-07-17 14:34 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch for working ICO encoder (40.73 KB, patch)
2011-07-18 11:45 PDT, Brian R. Bondy [:bbondy]
joe: review-
Details | Diff | Review
Patch for working ICO encoder + review comments (41.25 KB, patch)
2011-07-26 18:07 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
reftests for BMP and ICO encoders (44.06 KB, patch)
2011-07-26 18:10 PDT, Brian R. Bondy [:bbondy]
joe: review+
Details | Diff | Review
Patch for working ICO encoder + review comments (41.12 KB, patch)
2011-08-04 09:23 PDT, Brian R. Bondy [:bbondy]
joe: review+
Details | Diff | Review
reftests for BMP and ICO encoders (44.02 KB, patch)
2011-08-04 18:41 PDT, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Review
reftests for BMP and ICO encoders (44.03 KB, patch)
2011-08-04 18:50 PDT, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Review
Patch for working ICO encoder + review comments (41.79 KB, patch)
2011-08-16 13:58 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch for working ICO encoder + review comments (51.03 KB, patch)
2011-08-16 16:50 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch for working ICO encoder + review comments (41.83 KB, patch)
2011-08-16 16:58 PDT, Brian R. Bondy [:bbondy]
joe: review+
Details | Diff | Review

Description Jim Mathies [:jimm] 2010-03-01 15:21:49 PST
We'll need this for win7 jump list fave icon support. Icons on uri displayed in the lists need to point to local ico files, which we'll have to cache for the list under the app data dir or similar. For this we'll need the ability to write fave icons out as ico files.
Comment 1 Rimas Kudelis 2010-10-19 13:52:08 PDT
One way to simplify this would be to save PNG-encoded ICO files, which are supported by Vista and later. I think this would let "outsource" most of the work to the PNG encoder. And would result in smaller (but incompatible with XP) .ico files.

I've filed bug 600556 asking for support of rendering PNG-encoded ICO files a few weeks ago.
Comment 2 Brian R. Bondy [:bbondy] 2011-07-09 13:49:29 PDT
I added a new task which should be implemented before implementing the .ico encoder.

Bug 670466 - Add basic support for .bmp encoder
I'll start on Bug 670466 Monday and then when that is done will do this task after that.
Comment 3 Brian R. Bondy [:bbondy] 2011-07-11 13:14:56 PDT
Just to recap the task:

Favicons usually have ICO format because it is the only supported format for IE, but sometimes websites will use other formats.
Favicons for Firefox are supported with: ICO, PNG, GIF, JPEG, PNG, or SVG (possibly more).

Jumplist entries must be in ICO format.
We therefore need to be able to write out ICO files from images which are not in ICO format (in the cases where different favicon formats were used by sites).
To do this we need to implement an ICO encoder modules/libpr0n/encoders. (Currently we only have encoders for jpeg and png, but not ICO.)

Here is an example with a PNG favicon:
<html>
<head>
<link rel="icon" type="image/png" 
 href="http://example.com/image.png" />
</head>
<body></body>
</html>

I am implementing the ICOs with a contained BMP since if we need shell integration in older Windows OS that will be supported and there is no benefit to using 16x16 PNGs in size.
Comment 4 Rimas Kudelis 2011-07-11 13:31:17 PDT
(In reply to comment #3)
> I am implementing the ICOs with a contained BMP since if we need shell
> integration in older Windows OS that will be supported and there is no
> benefit to using 16x16 PNGs in size.

I think it may be better to go with PNG's though, because:
* the PNG encoder already exists in the code. You could plug it in, right?
* jumplists are only supported in Windows 7, while PNG icons are supported since Vista
* even though PNG icons are not supported in XP, Microsoft does its best to phase out XP soon anyway. And even considering its current popularity, the fact, that we have been living without an ICO encoder all the time, indicates that there hasn't been much demand for it in older versions of Windows anyway. Or am I wrong? :)
Comment 5 Brian R. Bondy [:bbondy] 2011-07-11 13:34:16 PDT
I'm not sure if we plan to do any kind of shell integration with FF pinned tabs or not.  In any case there is the ability to use encoder options so I think I'll support both ways. 

Also data can be exported to the OS with <canvas> now so people could export a file that doesn't work for PNG ICO, then they'd probably file a bug report.
Comment 6 Rimas Kudelis 2011-07-11 13:39:29 PDT
You mean someone may want to right-click on a canvas element and save it as an ico file? I personally would not expect Firefox to support exporting as ICO at all. It might be a cool feature, but I don't think it would be used often enough to justify its existence.
Comment 7 Brian R. Bondy [:bbondy] 2011-07-11 13:43:50 PDT
With the introduction of an ICO encoder you get support for <canvas>.toDataURL automatically. With that function anyone can specify a mime type.  For unsupported mime types it just returns PNG as per spec of the data URI scheme.  

With the ICO encoder it will now actually export ICOs.
It might not be used often but its possible to do.  
Also it is with that function that I'm creating ref tests to ensure we don't have a buggy encoder.  

As mentioned though I'll support both PNG and BMP ICOs, so it's nothing to worry about.  The user of the encoder can pick what to use.
Comment 8 Brian R. Bondy [:bbondy] 2011-07-11 19:39:47 PDT
Created attachment 545308 [details]
Intermediate patch for .ico icon encoder

Attached is an intermediate patch for an ICO encoder that supports PNG ICOs, 24BPP, and 32BPP.

Tomorrow I will finish up and also create ref tests.
Comment 9 Brian R. Bondy [:bbondy] 2011-07-11 19:43:57 PDT
Created attachment 545310 [details] [diff] [review]
Intermediate patch for .ico icon encoder

(Forgot to check on patch in the last message)
Comment 10 Brian R. Bondy [:bbondy] 2011-07-12 10:12:09 PDT
I added a patch to Bug 670466 which contains the reftests for the ICO encoder.  Please do not run them though until I submit a new code patch which fixes some of the tests that currently fail the reftests.
Comment 11 Brian R. Bondy [:bbondy] 2011-07-12 20:29:27 PDT
Created attachment 545583 [details] [diff] [review]
Finalized patch for ico encoder

Implemented the encoder which supports PNG, BMP32BPP, and BMP24BPP.  All reftests I made currently pass for it and the BMP encoder. 

I will wait to implement the review comments on the decoder before asking for a review here.
Comment 12 Brian R. Bondy [:bbondy] 2011-07-14 12:56:18 PDT
Created attachment 545981 [details] [diff] [review]
Finalized patch for ico encoder

minor update
Comment 13 Brian R. Bondy [:bbondy] 2011-07-17 14:34:38 PDT
Created attachment 546444 [details] [diff] [review]
Patch for working ICO encoder

Minor fixes so that the patch works with its dependant Bug 600556

I will wait for r? until I do some extra refactoring/commenting on the patch.
Comment 14 Brian R. Bondy [:bbondy] 2011-07-18 11:45:29 PDT
Created attachment 546587 [details] [diff] [review]
Patch for working ICO encoder

Some code refactoring and commenting, ready for review.
Comment 15 Joe Drew (not getting mail) 2011-07-25 12:44:20 PDT
Comment on attachment 546587 [details] [diff] [review]
Patch for working ICO encoder

Review of attachment 546587 [details] [diff] [review]:
-----------------------------------------------------------------

My major objection is ContainableEncoder, which requires multiple inheritance and (right now) defines a symbol that's already defined in a separate interface, which is a no-no. I think your best bet might be to extend the mozilla::imagelib::Decoder interface instead.

Also, I think you'll want to reimport your license boilerplate for all the new files from http://www.mozilla.org/MPL/boilerplate-1.1/ - just in case. I found some errors from copy and pasting old headers that makes me worry that I missed other license errors. Note also that the initial developer is the Mozilla Foundation. http://weblogs.mozillazine.org/gerv/archives/2010/02/mpl_initial_developer_for_mozilla_employ.html

::: modules/libpr0n/build/nsImageModule.cpp
@@ +101,5 @@
>    { "@mozilla.org/image/loader;1", &kNS_IMGLOADER_CID },
>    { "@mozilla.org/image/request;1", &kNS_IMGREQUESTPROXY_CID },
>    { "@mozilla.org/image/tools;1", &kNS_IMGTOOLS_CID },
>    { "@mozilla.org/image/rasterimage;1", &kNS_RASTERIMAGE_CID },
> +  { "@mozilla.org/image/encoder;2?type=image/ico", &kNS_ICOENCODER_CID },

This isn't actually the right MIME type for icons; the officially registered MIME type is image/vnd.microsoft.icon, though image/x-icon is also pretty common.

It doesn't actually matter that much, especially not for encoders, but I wanted to make sure this was officially noted.

::: modules/libpr0n/encoders/ico/Makefile.in
@@ +13,5 @@
> +#
> +# The Original Code is mozilla.org code.
> +#
> +# The Initial Developer of the Original Code is Mozilla Foundation
> +# Portions created by the Initial Developer are Copyright (C) 2005

2011 :)

::: modules/libpr0n/encoders/ico/nsICOEncoder.cpp
@@ +14,5 @@
> + *
> + * The Original Code is ICO Encoding code
> + *
> + * The Initial Developer of the Original Code is
> + * Google Inc.

No sir!

@@ +55,5 @@
> +                               mImageBufferSize(0), mImageBufferUsed(0), 
> +                               mImageBufferReadPoint(0), mCallback(nsnull),
> +                               mCallbackTarget(nsnull), mNotifyThreshold(0),
> +                               mUsePNG(PR_TRUE),
> +                               mReentrantMonitor("nsICOEncoder.mReentrantMonitor")

Quibble - I often prefer
Foo::Foo()
 : mFoo(0),
   mBar(1),

(or alternately 
 : mFoo(0)
 , mBar(1)
)

@@ +69,5 @@
> +  }
> +}
> +
> +// nsICOEncoder::InitFromData
> +// Two output options are supported: bpp=<bpp_value>;png=<yes|no>

I'd rather format=<png|bmp>

@@ +90,5 @@
> +  // Stride is the padded width of each row, so it better be longer
> +  if ((aInputFormat == INPUT_FORMAT_RGB &&
> +    aStride < aWidth * 3) ||
> +    ((aInputFormat == INPUT_FORMAT_RGBA || aInputFormat == INPUT_FORMAT_HOSTARGB) &&
> +    aStride < aWidth * 4)) {

Here, you should vertically align aStride such that it's right below aInputFormat, because it's part of the same bracketed clause. Similarly, make sure the first ( in ((aInputFormat is vertically aligned directly below the second ( in if ((aInputFormat.

Basically, make sure indentation follows bracket depth.

Do this throughout, please.

@@ +114,5 @@
> +NS_IMETHODIMP 
> +nsICOEncoder::AddImageFrame(const PRUint8* aData,
> +  PRUint32 aLength, // (unused,
> +  // req'd by JS)
> +  PRUint32 aWidth,

Can you fix the formatting of this function's parameters?

@@ +126,5 @@
> +    mContainedImage = new nsPNGEncoder();
> +    nsresult rv;
> +    nsAutoString noParams;
> +    rv = mContainedImage->InitFromData(aData, aLength, aWidth, aHeight,
> +      aStride, aInputFormat, noParams);

Can you vertically align aStride here with aData?

vim has a nice configuration setting to let you do this:

set cino=(0,w1

(Do this throughout everything)

@@ +133,5 @@
> +      return rv;
> +
> +    mImageBufferSize = ICONFILEHEADERSIZE + ICODIRENTRYSIZE + 
> +                       mContainedImage->GetImageBufferSize();
> +    mImageBufferStart = (PRUint8*)PR_Malloc(mImageBufferSize);

Why PR_Malloc?

@@ +229,5 @@
> +    return NS_ERROR_INVALID_ARG;
> +
> +  // parse and check any provided output options
> +  PRUint32 bpp;
> +  PRBool usePNG;

You should probably initialize these in this function instead of relying on ParseOptions to initialize them.

@@ +232,5 @@
> +  PRUint32 bpp;
> +  PRBool usePNG;
> +  nsresult rv = ParseOptions(aOutputOptions, &bpp, &usePNG);
> +  if (rv != NS_OK)
> +    return rv;

NS_SUCCEEDED(rv) instead of != NS_OK.

@@ +278,5 @@
> +  nsCAutoString optionsCopy;
> +  optionsCopy.Assign(NS_ConvertUTF16toUTF8(aOptions));
> +  char* options = optionsCopy.BeginWriting();
> +
> +  while (char* token = nsCRT::strtok(options, ";", &options)) {

I think it might be a bit cleaner if you used ParseString or FindInReadable (see nsReadableUtils.h) rather than strtok. See also https://developer.mozilla.org/en/XPCOM_string_guide

At a bare minimum, though, you should switch to NS_strtok.

@@ +363,5 @@
> +                                         void *aClosure, PRUint32 aCount,
> +                                         PRUint32 *_retval)
> +{
> +  // Avoid another thread reallocing the buffer underneath us
> +  ReentrantMonitorAutoEnter autoEnter(mReentrantMonitor);

Did this happen to you? Imagelib is generally pretty aggressively single-threaded.

@@ +477,5 @@
> +  mICODirEntry.mBitCount = aBPP;
> +  mICODirEntry.mBytesInRes = 0;
> +  mICODirEntry.mColorCount = 0;
> +  mICODirEntry.mWidth = aWidth;
> +  mICODirEntry.mHeight = aHeight;

aWidth and aHeight are PRUint32s, but mWidth and mHeight are PRUint8s. Give careful thought to what happens here if the PRUint32s are too big for the PRUint8s!

::: modules/libpr0n/encoders/ico/nsICOEncoder.h
@@ +13,5 @@
> + * License.
> + *
> + * The Original Code is Oracle Corporation code.
> + *
> + * The Initial Developer of the Original Code is Oracle Corporation.

Hmm....

@@ +85,5 @@
> +  void EncodeInfoHeader();
> +
> +  // Holds either a PNG or a BMP depending on the encoding options specified
> +  // or if no encoding options specified will use the default (PNG)
> +  nsAutoPtr<mozilla::imagelib::ContainableEncoder> mContainedImage;

This should probably be called mContainedEncoder.

Also, you could add "using mozilla::imagelib" and s/mozilla::imagelib::// throughout.

::: modules/libpr0n/src/ContainableEncoder.h
@@ +15,5 @@
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + * the Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2010.

2011

@@ +46,5 @@
> +{
> +public:
> +  virtual PRUint32 GetImageBufferSize() const = 0;
> +  virtual PRUint8* GetImageBuffer() const = 0;
> +  NS_IMETHOD InitFromData(const PRUint8* aData,

It's quite weird to have an NS_IMETHOD (which means that it's an IDL-defined interface method) defined in a regular class. You might be a bit happier if you create a separate method that just calls the imgIEncoder version (implemented either in ContainableEncoder or in the concrete classes, it doesn't matter which).
Comment 16 Brian R. Bondy [:bbondy] 2011-07-25 12:56:12 PDT
Thanks for the review, I'll:
- Extend mozilla::imagelib::Decoder interface instead of having the multiple inheritance and nuke ContainableEncoder. 
- Use image/vnd.microsoft.icon
- Fixup licensing at top of files and pay attention to that next time I create a new file. 
- Change to format=<png|bmp>
- Do some kind of error checking when width and height are out of bounds. 
- Fixup formatting and other comments
Comment 17 Brian R. Bondy [:bbondy] 2011-07-26 11:24:27 PDT
> > ReentrantMonitorAutoEnter autoEnter(mReentrantMonitor);
> Did this happen to you? Imagelib is generally pretty aggressively single-threaded.

It did not happen to me but I put it the same as the PNG and JPEG encoders.  Maybe I can add another task to remove from all encoders separately so it stays consistent with the other encoders?
Comment 18 Brian R. Bondy [:bbondy] 2011-07-26 18:07:01 PDT
Created attachment 548648 [details] [diff] [review]
Patch for working ICO encoder + review comments

All comments implemented and formatting fixed.
Comment 19 Brian R. Bondy [:bbondy] 2011-07-26 18:10:11 PDT
Created attachment 548650 [details] [diff] [review]
reftests for BMP and ICO encoders

This reftest patch was previously r+'d in Bug 670466.
- I changed the formatting options to format=png|bmp to match the code
- I changed the mime type to the proper ico encoding to match the code
- I moved the reftest patch to this bug because it contains both ICO and BMP tests, and this bug (ico encoder) comes after Bug 670466 (bmp encoder)
Comment 20 Brian R. Bondy [:bbondy] 2011-08-04 09:23:30 PDT
Created attachment 550723 [details] [diff] [review]
Patch for working ICO encoder + review comments

Implemented Review comments from Bug 670466 but applied to this bug.
Comment 21 Joe Drew (not getting mail) 2011-08-04 12:25:49 PDT
Comment on attachment 548650 [details] [diff] [review]
reftests for BMP and ICO encoders

Review of attachment 548650 [details] [diff] [review]:
-----------------------------------------------------------------

Needs a couple of minor updates, but overall looks good. I don't need to review this again.

::: modules/libpr0n/test/reftest/encoders-lossless/encoder.html
@@ +13,5 @@
> +  // - options=<canvas toDataURL encoder options>
> +  // Example: 
> +  // encoder.html?img=escape(reference_image.png)
> +  //             &mime=escape(image/ico)
> +  //             &options=escape(-moz-parse-options:bpp=24;png=no)

Should probably update this example of -moz-parse-options for format=<png|bmp> if that's what we're going with. (Not that it's that important, as it's just an example.)

@@ +28,5 @@
> +    }
> +    if (isFinite(Date.parse(sValue))) { 
> +      return(new Date(sValue)); 
> +    }
> +    return(sValue);

Return isn't a function, so you don't need to put brackets around the things you're returning.

@@ +63,5 @@
> +
> +  // Once the image loads async, we then draw the image onto the canvas,
> +  // and call canvas.toDataURL to invoke the encoder, and then set a new
> +  // image to the encoded data URL
> +  function onReferenceImageLoad() {

function openbrace goes on its own line.

@@ +73,5 @@
> +    // Obtain the canvas and draw the reference image
> +    canvas.width = img.width;
> +    canvas.height = img.height;
> +    var ctx = canvas.getContext('2d')
> +    ctx.drawImage(img, 0, 0, img.width, img.height);

You don't need to specify img.width and img.height here, since drawImage defaults to that.

@@ +77,5 @@
> +    ctx.drawImage(img, 0, 0, img.width, img.height);
> +
> +    // Obtain the data URL with parsing options if specified
> +    var dataURL;
> +    if(parseOptions && parseOptions.length > 0) 

I checked, and the empty string is interpreted as false, so you don't need parseOptions.length > 0 here. Also, you can put a space between if and ( throughout.
Comment 22 Brian R. Bondy [:bbondy] 2011-08-04 18:38:28 PDT
> Return isn't a function...

Yes I know :) It was leftover from the previous reference you gave me for getting URL parameters: https://developer.mozilla.org/en/window.location
Comment 23 Brian R. Bondy [:bbondy] 2011-08-04 18:41:17 PDT
Created attachment 550925 [details] [diff] [review]
reftests for BMP and ICO encoders

Implemented review comments on reftest patch
Comment 24 Brian R. Bondy [:bbondy] 2011-08-04 18:50:11 PDT
Created attachment 550927 [details] [diff] [review]
reftests for BMP and ICO encoders

Fixed an additional comment in reftest.list which had the old example png=yes instead of format=png.
Comment 25 Joe Drew (not getting mail) 2011-08-15 14:00:19 PDT
Comment on attachment 550723 [details] [diff] [review]
Patch for working ICO encoder + review comments

Review of attachment 550723 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Some small nits below.

::: modules/libpr0n/encoders/bmp/nsBMPEncoder.cpp
@@ +184,5 @@
> +// Returns a pointer to the start of the image buffer
> +NS_IMETHODIMP nsBMPEncoder::GetImageBuffer(char **aOutputBuffer)
> +{
> +  NS_ENSURE_ARG_POINTER(aOutputBuffer);
> +  *aOutputBuffer = (char*)mImageBufferStart;

reinterpret_cast maybe? (For all encoders, do the same thing, anyways; right now we use reinterpret_cast in the ICO encoder and C-style casts everywhere else.)

::: modules/libpr0n/encoders/ico/nsICOEncoder.cpp
@@ +150,5 @@
> +    PRUint32 imageBufferSize;
> +    mContainedEncoder->GetImageBufferSize(&imageBufferSize);
> +    mImageBufferSize = ICONFILEHEADERSIZE + ICODIRENTRYSIZE + 
> +                       imageBufferSize;
> +    mImageBufferStart = static_cast<PRUint8*>(moz_malloc(mImageBufferSize));

new (fallible_t()) PRUint8[mImageBufferSize] might be prettier? I don't know really.

@@ +178,5 @@
> +                                         aStride, aInputFormat, params);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    PRUint32 andMaskSize = ((mICODirEntry.mWidth + 31) / 32) * 4 * // row AND mask
> +                           mICODirEntry.mHeight; //num rows

space between // and num here

@@ +205,5 @@
> +           imageBufferSize - BFH_LENGTH);
> +    // We need to fix the BMP height to be *2 for the AND mask
> +    PRUint32 fixedHeight = mICODirEntry.mHeight * 2;
> +    fixedHeight = NATIVE32_TO_LITTLE(fixedHeight);
> +    memcpy(mImageBufferCurr + 8, &fixedHeight, sizeof(fixedHeight));

This bare "+ 8" makes me a little uneasy - what's its derivation?

@@ +536,5 @@
> +
> +  memcpy(mImageBufferCurr, &littleEndianmIDE.mWidth, sizeof(littleEndianmIDE.mWidth));
> +  memcpy(mImageBufferCurr + 1, &littleEndianmIDE.mHeight, sizeof(littleEndianmIDE.mHeight));
> +  memcpy(mImageBufferCurr + 2, &littleEndianmIDE.mColorCount, sizeof(littleEndianmIDE.mColorCount));
> +  memcpy(mImageBufferCurr + 3, &littleEndianmIDE.mReserved, sizeof(littleEndianmIDE.mReserved));

It might be better to have a separate pointer that gets incremented by sizeof(...) every time, so we don't have all these bare numbers inside. (Same goes for EncodeFileHeader.)
Comment 26 Brian R. Bondy [:bbondy] 2011-08-15 14:01:46 PDT
> It might be better to have a separate pointer that gets incremented by sizeof

I had a different one originally but you made me remove it :)
I think you mean a local one though and not a member like I had before right?
Comment 27 Brian R. Bondy [:bbondy] 2011-08-15 14:02:49 PDT
ah never mind I was thinking of a different spot in code.   My bad! :)
Comment 28 Joe Drew (not getting mail) 2011-08-15 14:03:07 PDT
Right - a local one. :)
Comment 29 Brian R. Bondy [:bbondy] 2011-08-16 07:01:24 PDT
> new (fallible_t()) PRUint8[mImageBufferSize] might be prettier? 

I tried like that originally but it won't compile that way.  The other references do it the same way as me as well in the code base of mozilla-central.  (Function cast definitions illegal)
Comment 30 Brian R. Bondy [:bbondy] 2011-08-16 13:58:22 PDT
Created attachment 553576 [details] [diff] [review]
Patch for working ICO encoder + review comments

Implemented latest review comments.
Comment 31 Joe Drew (not getting mail) 2011-08-16 14:39:08 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #29)
> > new (fallible_t()) PRUint8[mImageBufferSize] might be prettier? 
> 
> I tried like that originally but it won't compile that way.  The other
> references do it the same way as me as well in the code base of
> mozilla-central.  (Function cast definitions illegal)

Yeah, it turns out that MSVC has a bug that makes that not work. (Bug 679503 is going to fix that.) In the mean time, you can say something along the lines of 

  static fallible_t fallible = fallible_t();
  Block* b = new (fallible) Block();

like in bug 613766 to work around the problem.
Comment 32 Brian R. Bondy [:bbondy] 2011-08-16 16:44:26 PDT
The fallible change from static to const is going to be in Bug 670466, so the attached patch is ready for review whenever you get a chance.
Comment 33 Brian R. Bondy [:bbondy] 2011-08-16 16:50:00 PDT
Created attachment 553628 [details] [diff] [review]
Patch for working ICO encoder + review comments
Comment 34 Brian R. Bondy [:bbondy] 2011-08-16 16:54:55 PDT
hold on this one I uploaded wrong patch :)
Comment 35 Brian R. Bondy [:bbondy] 2011-08-16 16:58:27 PDT
Created attachment 553632 [details] [diff] [review]
Patch for working ICO encoder + review comments
Comment 36 Brian R. Bondy [:bbondy] 2011-08-16 16:59:17 PDT
Good to go.

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